Skip to content

Conversation

@andreped
Copy link
Collaborator

I accidentally implemented support for TensorFlow backend.

Changes:

  • Added TensorFlow alternative with equivalent classes and functions which seem to work just fine
  • Added TF example in examples.py, along with runtime results for all three
  • Minor refactoring of the logic for the backend selection
  • ** Removed unecessary inheritance from the HENormalizer class
  • *** Added least squares tensorflow implementation relevant to solve a singular matrix problem (which torch did by default).

** Up to you if you find that relevant, if yes, it would be of interest to know why.
*** Could be made more generic, but it is not necessary for this to work.

@andreped
Copy link
Collaborator Author

There might be some potential conflicts between torch and tensorflow when installing.

I haven't had time to explore and verify this myself, but I have a hunch.

Perhaps some of the devs might have the time, @carloalbertobarbano?

@carloalbertobarbano
Copy link
Member

Hi André, thanks a lot again for your contribution. Everything seems ok, and I will accept it.

I would only leave the inheritance from the HENormalizer class. It's a design choice rather than an actual need (e.g. you can use typing hints such as normalizer: HENormalizer)

I will look into the potential installation issues

@andreped
Copy link
Collaborator Author

I added back the HENormalizer class inheritance to both the torch and tf normalization classes.

Have you gotten any results from testing installs with both TF and Torch?

@carloalbertobarbano
Copy link
Member

Thanks a lot.
I still haven't tried installing the new version. I was thinking maybe to remove both the torch and the tf requirements, and let the user choose which one to install in order to minimize unnecessary dependencies.

@carloalbertobarbano carloalbertobarbano merged commit b466869 into EIDOSLAB:main Sep 22, 2021
@andreped
Copy link
Collaborator Author

I also like giving the user the option to choose which backend to use when installing, as it avoids bloatware. However, I would think that if the user wishes to use e.g. the tensorflow backend, it should be defined when pip installing with an argument (or similar). This is quite easy to do. You just need to set this up with the setup.py file, to support said argument, and modify the requirements to be installed there.

One shouldn't force the user to manually having to install TF or Torch, before installing torchstain. Thats annoying. You agree?

Anyway, when this is done, I think it is time for a new release :)

@carloalbertobarbano
Copy link
Member

Yes, i think this could be the best solution. I will work on it as soon as possible, thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants