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

Fix train_model.py bug, fix Analysis module import #39

Merged
merged 3 commits into from
Oct 19, 2022
Merged

Conversation

smericks
Copy link
Collaborator

  • Fix bug for params_as_inputs in train_model.py
  • Fix relative import in train_model.py
  • Fix import of Analysis module in init.py

@smericks
Copy link
Collaborator Author

smericks commented Oct 3, 2022

@swagnercarena I tried to reproduce the failed test (test_plot_coverage() in analysis tests), but I'm passing all tests on my machine. Any idea what's causing this?

Copy link
Collaborator

@JelleAalbers JelleAalbers left a comment

Choose a reason for hiding this comment

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

Looks good Sydney, thanks for fixing! Maybe you'd like to do if params_as_inputs instead of if len(params_as_inputs) > 0; that's slightly more concise and avoids crashing on values like None or False (though I don't see how those could easily arise in this code).

I saw the same test failure; I think it is due to a new matplotlib version. So we'd see the same on master if we were to redo the build there. #38, and in particular 94bfd3d, will fix this.

@smericks
Copy link
Collaborator Author

smericks commented Oct 7, 2022

Yes I agree that's a cleaner way to do it, I just made that change.

Ok I see, glad there's a fix on the way!

Copy link
Collaborator

@JelleAalbers JelleAalbers left a comment

Choose a reason for hiding this comment

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

Thanks Sydney!

@JelleAalbers
Copy link
Collaborator

@swagnercarena Do you agree this is good to merge? Sorry, I should have asked you explicitly before approving.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 3285470493

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 95.044%

Totals Coverage Status
Change from base Build 2702188126: -0.03%
Covered Lines: 2282
Relevant Lines: 2401

💛 - Coveralls

@swagnercarena
Copy link
Owner

Thanks for making these additions @smericks. I went ahead and included @JelleAalbers fix so that we didn't get failures on the main page (I also included a 0.1 decrease coverage threshold for failure since the 0% threshold was too strict).

@swagnercarena swagnercarena merged commit 2edd7f4 into main Oct 19, 2022
padma18-vb pushed a commit to padma18-vb/paltas that referenced this pull request Jan 24, 2024
Fix train_model.py bug, fix Analysis module import
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.

4 participants