Remove deprecated arguments from SN model initialisers #356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
Sheshuk
reviewed
Aug 14, 2024
There was a problem hiding this comment.
Changes look good so far, and we're heading towards a cleaner and more beautiful future interface!
Looks like you've got everything, concerning with legacy_filename_initialization.
I agree with the plan to remove the deprecated parameters and putting them directly into the metadata.
Base automatically changed from
JostMigenda/variousDeprecations
to
release_v2.0
August 14, 2024 14:51
|
Ready for review now! |
|
Oh, the integration tests are failing slightly earlier now; I’ll put in a fix for that in a minute … |
|
Okay, now the integration tests are back to failing in |
sybenzvi
approved these changes
Aug 14, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is intended to remove deprecations in the SN model initialisers, including initialising from a file name and inconsistent keyword arguments.
There are still a couple of
@deprecateddecorators we should remove before merging this. (E.g. for many models, theeoskeyword is marked as deprecated, because there is only a single possible value; so it shouldn’t be required as part of the__init__.)I think the best way to proceed there would be to remove
eosfrom the parameter list and manually setself.metadata["eos"]in the__init__to the fixed value instead. I’ll do that tomorrow and mark this as a draft PR until then; but wanted to get it out now before I go to bed, in case you want to review this during your daytime, @Sheshuk. (Yay for timezones! 😉)(Also: Yes, I’ve currently marked the branch from #353 as a base for this PR for simplicity; but can rebase it onto the
release_v2.0branch tomorrow.)