Skip to content

fix: features transform in Trainer._save_model internal method#573

Merged
gcroci2 merged 3 commits intodevfrom
570_fix_features_transform_gcroci2
Feb 21, 2024
Merged

fix: features transform in Trainer._save_model internal method#573
gcroci2 merged 3 commits intodevfrom
570_fix_features_transform_gcroci2

Conversation

@gcroci2
Copy link
Copy Markdown
Collaborator

@gcroci2 gcroci2 commented Feb 21, 2024 •
edited
Loading

  • The bug was due to the quotation marks type used in the regex for extracting the transform function. The quotation marks are indeed now automatically fixed to double type by ruff in our VSCode editor. Since a user should be able to use both types of quotation marks, I changed the regex that matches the transform function for handling both cases. The bug was causing the failure of the training.ipynb notebook.
  • I also added a test for checking on the features_transform attribute in the Trainer, and in case we use a pre-trained model. The testing on the trainer side for features_transform was missing.

As soon as this is approved, I will release a new patch version for deeprank2.

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 21, 2024

Coverage Status

coverage: 83.709% (+0.1%) from 83.613%
when pulling 2c7f8eb on 570_fix_features_transform_gcroci2
into c45f828 on main.

@gcroci2 gcroci2 changed the base branch from main to dev February 21, 2024 12:51
@gcroci2 gcroci2 linked an issue Feb 21, 2024 that may be closed by this pull request
Fix features_transform AttributeError in Trainer._save_model method #570
Closed
2 tasks
@gcroci2 gcroci2 requested a review from DaniBodor February 21, 2024 15:02
DaniBodor
DaniBodor approved these changes Feb 21, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@DaniBodor DaniBodor left a comment

Choose a reason for hiding this comment

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

Nice catch, once again.

If ever there is some extra time on this project (not likely), it would be nice to refactor the entire tests folder. Many of these modules are getting really long and confusing and contain lots of duplicate code. It's hard to keep track of what is going on. A nice cleanup could make it really easy to quickly assess what each test does and how.

@gcroci2
Copy link
Copy Markdown
Collaborator Author

gcroci2 commented Feb 21, 2024

Nice catch, once again.

If ever there is some extra time on this project (not likely), it would be nice to refactor the entire tests folder. Many of these modules are getting really long and confusing and contain lots of duplicate code. It's hard to keep track of what is going on. A nice cleanup could make it really easy to quickly assess what each test does and how.

Indeed, it would be very nice. We also have already issues for that. Let's see what budgets we get ;)

@gcroci2 gcroci2 merged commit 4d29971 into dev Feb 21, 2024
@gcroci2 gcroci2 deleted the 570_fix_features_transform_gcroci2 branch February 21, 2024 15:34
@gcroci2 gcroci2 mentioned this pull request Feb 22, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DaniBodor DaniBodor DaniBodor approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fix features_transform AttributeError in Trainer._save_model method

3 participants

@gcroci2 @coveralls @DaniBodor