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 Harmonization #130

Merged
merged 40 commits into from
Mar 1, 2023
Merged

add Harmonization #130

merged 40 commits into from
Mar 1, 2023

Conversation

candemircan
Copy link
Contributor

Adding a custom Harmonization class. The model is explained in this paper and is available under this repo. It is a type of model that gets deep neural networks to rely on similar aspects of images as humans do while doing object recognition.

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 19, 2023

@candemircan Thanks for this PR! I think this will definitely be a useful addition to the current models in thingsvision. I know Drew's work quite well. Could you add tests for at least two of the models, and update the README.md similarly to how you've already changed the docs? I think you need to rebase before you do any of the above (due to recent changes in the codebase).

@candemircan
Copy link
Contributor Author

hey Lukas :) thanks for the quick response. Just made the changes as you requested. Hope it works out fine!

Best,
Can

@LukasMut
Copy link
Collaborator

@candemircan, thanks a lot for the changes! Could you rebase one more time and submit the PR again? I had to accept a different PR before I could accept yours.

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 19, 2023

@candemircan Also, could you do me a favor and remove the folder build/lib after you've rebased? This folder was accidentally added during the previous PR but shouldn't part of the the repo.

auto-merge was automatically disabled February 20, 2023 10:50

Head branch was pushed to by a user without write access

@LukasMut
Copy link
Collaborator

@candemircan I see that the unittests are not passing. Did you rebase correctly?

@candemircan
Copy link
Contributor Author

@LukasMut I believe I did, but a comma was missing. Now I fixed it. Let's see if it passes now

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 20, 2023

@candemircan Unfortunately, the unittests are still failing. I think there's a module missing for one of the harmonization models,

ModuleNotFoundError: No module named 'keras_cv_attention_models.maxvit'

Please see yourself when and why the tests are failing.

auto-merge was automatically disabled February 20, 2023 13:23

Head branch was pushed to by a user without write access

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 20, 2023

@candemircan I see that you've added the missing module to the requirements.txt file, but I am wondering whether it has to be added to the setup.py and environment.yml files too? Unfortunately, the tests are still failing for some reason. Could you test locally what's going on?

@candemircan
Copy link
Contributor Author

candemircan commented Feb 20, 2023 via email

@candemircan
Copy link
Contributor Author

candemircan commented Feb 25, 2023

Hi again Lukas,

I have added the keras_cv_attention_models to requirements.txt and setup.py. I see that the dependency is also installed under GitHub Actions, however the mac-os test still fails with the same ModuleNotFoundError: No module named 'keras_cv_attention_models.maxvit' message. Do you have any idea why this may still be the case?

Best,
Can

@candemircan candemircan reopened this Feb 25, 2023
@LukasMut
Copy link
Collaborator

LukasMut commented Feb 25, 2023

@Alxmrphi, do you have any idea what's going on with codecov here?

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 25, 2023

@Alxmrphi, do you have any idea what's going on with codecov here?

My gut feeling tells me that it has something to do with the TensorFlow version, but I am not 100% sure. One could also try to pip install git+https://github.com/leondgarse/keras_cv_attention_models rather than keras-cv-attention-models.

@candemircan do you use keras-cv-attention-models or keras_cv_attention_models for installing the module via pip? I think it needs to be the former (although the Harmonization repo uses the latter).

What's interesting is that the unittests are both passing but codecov is failing 🤔

@Alxmrphi
Copy link
Collaborator

I'm scratching my head at this one, too. How is it passing the unit tests but not the code coverage? I downloaded and ran the unit tests locally and they fail on the harmonization model tests. Quick fixes to install libraries, more failures in different places. The first hurdle it fails at is not having access to keras_applications.

@LukasMut LukasMut added enhancement New feature or request question Further information is requested labels Feb 27, 2023
@candemircan
Copy link
Contributor Author

@candemircan do you use keras-cv-attention-models or keras_cv_attention_models for installing the module via pip? I think it needs to be the former (although the Harmonization repo uses the latter).

Hi Lukas,

I was using the latter, but you are right that the package is named as the former in pypi. Should I change it and make another commit?

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 27, 2023

@candemircan do you use keras-cv-attention-models or keras_cv_attention_models for installing the module via pip? I think it needs to be the former (although the Harmonization repo uses the latter).

Hi Lukas,

I was using the latter, but you are right that the package is named as the former in pypi. Should I change it and make another commit?

@candemircan Let me see whether I can figure this out. I feel like that error is not happening on our end.

@LukasMut
Copy link
Collaborator

LukasMut commented Feb 27, 2023

I'm scratching my head at this one, too. How is it passing the unit tests but not the code coverage? I downloaded and ran the unit tests locally and they fail on the harmonization model tests. Quick fixes to install libraries, more failures in different places. The first hurdle it fails at is not having access to keras_applications.

@Alxmrphi, I see that this error is happening within the harmonization module and not on our end. See the issue that references this PR.

Copy link
Collaborator

@LukasMut LukasMut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve this PR but I'd like to wait until we've fixed the dependency issues before I merge it into the main branch.

Copy link
Collaborator

@andropar andropar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as Lukas, we should wait until we figured out what causes codecov to fail, but the rest of the PR looks fine to me!

@LukasMut LukasMut added this pull request to the merge queue Mar 1, 2023
Merged via the queue into ViCCo-Group:master with commit c96be31 Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants