Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve parameters and full_parameters #3509

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hersle
Copy link
Contributor

@hersle hersle commented Mar 27, 2025

"Fix" #3507, or at least try to prevent others from running into the same.

Is it breaking to rename the keyword argument initial_parameters to initial? I would prefer initial and dependent over initial_parameters and dependent_parameters. I think the latter is too verbose and would grow out of hand if more flags are added in the future.

@ChrisRackauckas
Copy link
Member

initial_parameters to initial?

It's brand new so it's in the range of time that it's like.. sure? If @TorkelE is onboard.

I do think making it all one function is just simpler API. Condense full_parameters and parameters. Condense equations and full_equations. Less functions, just different knobs and a good docstring.

@AayushSabharwal take it from here.

@ChrisRackauckas ChrisRackauckas requested review from SebastianM-C and AayushSabharwal and removed request for SebastianM-C March 27, 2025 11:56
@AayushSabharwal
Copy link
Member

Yep, looks good. I intentionally didn't document the initial_parameters kwarg. We're free to change it. It's also worth listing the "old" functions in #3204 so we remember to remove them later on.

@hersle
Copy link
Contributor Author

hersle commented Mar 27, 2025

I do think making it all one function is just simpler API. Condense full_parameters and parameters. Condense equations and full_equations. Less functions, just different knobs and a good docstring.

Yes, I fully agree. This PR is definitely in that direction. I can still see the use for one easy-to-use function to get all parameters/equations, though, which is equivalent to calling the "normal" getter with all flags enabled.

Copy link
Member

@AayushSabharwal AayushSabharwal left a comment

Choose a reason for hiding this comment

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

If we're making these flags public, they need tests

@hersle
Copy link
Contributor Author

hersle commented Mar 27, 2025

I

  1. added a test
  2. replaced the keyword argument initial_parameters by initial
  3. replaced the keyword argument add_initial_parameters by add_inital (in complete; for consistency with 2.)
  4. replaced the keyword argument dependent_parameters by dependent

I understand the name changes are subjective. I just feel these shorter names are more future-proof. Just let me know if I should revert any or all of them, and I am happy to do it.

@SebastianM-C
Copy link
Contributor

SebastianM-C commented Mar 27, 2025

IMO the longer names are more clear (as it makes it clear that we are talking about parameters), but I don't have a strong opinion here. Maybe initial_params or initial_ps would be an option?

@AayushSabharwal
Copy link
Member

The longer names can get a bit annoying, but I kind of feel like they should be plural? initials = true, dependents = true? That way it's very clear you're using a collective noun rather than an adjective.

@hersle
Copy link
Contributor Author

hersle commented Mar 27, 2025

I agree that initials would be better than initial etc. But I also see @SebastianM-C's point and don't want to change and stir things up unnecessarily, so I reverted the name changes. Sorry for the extra trouble 😅

@TorkelE
Copy link
Member

TorkelE commented Mar 27, 2025

Sounds good to me. Given that they are keargs to the "parameters" function, I don't mind not having "parameters" in the kwarg names as well.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants