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 code to copy instance methods from original original class defini… #81

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

deanpoulos
Copy link
Collaborator

Copy all callable attributes from the parameters instance to the Pydantic model that eventually replaces the parameters instance as QualibrationNode.parameters

@deanpoulos
Copy link
Collaborator Author

@maxim-v4s I can't run the formatting-checking code locally, but clearly see that what I wrote didn't make it happy. In addition to reviewing, could you please reformat and see that tests pass? Thanks

@nulinspiratie
Copy link
Contributor

@deanpoulos just to confirm, this should work even if the custom class is defined in a script, and it should work both when you run it locally and through the FE?

@nulinspiratie
Copy link
Contributor

@maxim-v4s some context for this PR: @deanpoulos want to add methods to a NodeParameters subclass. It seems that these methods aren't being copied, likely during the loading of the Parameters object

@maxim-v4s
Copy link
Collaborator

@nulinspiratie @deanpoulos
Should we copy staticmethods or classmethods? Or only instance level?
Should we copy methods of base classes?

@nulinspiratie
Copy link
Contributor

nulinspiratie commented Jan 27, 2025

@deanpoulos it seems that this PR introduces some breaking changes, specifically in situations where the original NodeParameters have required parameters (without a default value). In that case, loading a serialized object can raise an error.
I discussed this with @maxim-v4s, and agreed that a different strategy is probably needed. Specifically, loading should use the class of the passed parameter instance. This will be a more involved change. Let's discuss what the timeline should be for this feature

@deanpoulos
Copy link
Collaborator Author

@nulinspiratie I'm glad you picked up on some breaking changes. Since I don't know the ins-and-outs of QUAlibrate, and how difficult an alternative implementation would be, can I leave this to you to prioritize? Even if it goes to the bottom of the list, it's OK.

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.

3 participants