Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add modified reinhard support #30

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

andreped
Copy link
Collaborator

@andreped andreped commented Dec 22, 2022

This PR extends Reinhard and adds a new method argument, which enables the user to choose between traditional Reinhard and the Modified version. All backends supported.

This PR can be merged after #27 , as this branch was based on that branch.

@carloalbertobarbano carloalbertobarbano changed the base branch from main to development January 17, 2023 08:54
@carloalbertobarbano
Copy link
Member

Hi, I am finally back from vacation. I am merging all the pending PR, however I see some tests are failing due to FAILED tests/test_tf.py::test_macenko_tf - AttributeError: module 'numpy' has no attribute 'float' ..we are using np.float in conversions, which was removed starting from numpy 1.24.0. Should we provide a fix?

@carloalbertobarbano
Copy link
Member

carloalbertobarbano commented Jan 17, 2023

For Modified Reinhard, nice work. I was thinking maybe of refactoring it using a sublcass ModifiedReinhardNormalizer rather than the method argument, in order to keep consistency; but it's not strictly mandatory for now. What do you think?

@andreped
Copy link
Collaborator Author

andreped commented Jan 17, 2023

Should we provide a fix?

Definitely. I just did not observe that on my local testing setup, as I was using an older numpy version. Using np.float32 should also be backward compatible. Seems like you are making the necessary changes now in your own development branch?

For Modified Reinhard, nice work. I was thinking maybe of refactoring it using a sublcass ModifiedReinhardNormalizer rather than the method argument, in order to keep consistency; but it's not strictly mandatory for now. What do you think?

That was actually my first suggestion, but then I realized how extremely similar the two variants were, and I thought having a method flag makes more sense. But up to you :]


EDIT:
Also note that method arg might become relevant for other methods as well in the future, as methods like Macenko have modified variants themselves.

@carloalbertobarbano
Copy link
Member

Okay, let's keep the method argument for now. Yes, I will merge everything onto the development branch and then I will fix the numpy tests

@carloalbertobarbano carloalbertobarbano merged commit 9b8e6cc into EIDOSLAB:development Jan 17, 2023
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