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 Cost Alias Getters #1131

Merged
merged 2 commits into from
Jun 21, 2024
Merged

Add Cost Alias Getters #1131

merged 2 commits into from
Jun 21, 2024

Conversation

GabrielKS
Copy link
Collaborator

The main goal here, discussed with @rodrigomha, is to allow a user of the ValueCurve cost aliases to get everything they need without unwrapping to the FunctionData. I also cleaned up some docs and tests that were outdated due to a VOM cost refactor. I didn't fix this failure, which I assume is due to changes elsewhere:

Screenshot 2024-06-21 at 11 35 57 AM

Adding @kdayday as a reviewer of these added semantics:

  • get_slopes(::PiecewisePointwiseCurve) calculates the slopes from the underlying pointwise FunctionData
  • get_slopes(::PiecewiseIncrementalCurve) fetches the slopes == the y-coordinates of the underlying step FunctionData
  • get_average_rates(::PiecewiseAverageCurve) fetches the average rates == the y-coordinates of the underlying step FunctionData

@rodrigomha
Copy link
Contributor

I think we should have an interface directly to the CostCurve and FuelCurve (in addition to the interface to the value_curve), for example:

get_constant_term(cost_curve::CostCurve{LinearCurve}) = get_constant_term(get_value_curve(cost_curve))

What do you think @jd-lara @GabrielKS ?

Copy link
Contributor

@kdayday kdayday left a comment

Choose a reason for hiding this comment

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

Comments look good. Have you re-made to docs to see if these are showing up in the API or on the ValueCurve page? If not, maybe we can merge this into the docs branch when you're done where I'll also make that change to have a separate page for each valuecurve

@GabrielKS
Copy link
Collaborator Author

I think we should have an interface directly to the CostCurve and FuelCurve (in addition to the interface to the value_curve), for example:

get_constant_term(cost_curve::CostCurve{LinearCurve}) = get_constant_term(get_value_curve(cost_curve))

What do you think @jd-lara @GabrielKS ?

Mildly oppose, having so many methods only work on CostCurve if it is parameterized a certain way seems like bad design. Technically that is what I'm doing here too, but only because the user will be treating each cost alias as if it is a completely separate struct. But I could be convinced.

@GabrielKS
Copy link
Collaborator Author

Comments look good. Have you re-made to docs to see if these are showing up in the API or on the ValueCurve page? If not, maybe we can merge this into the docs branch when you're done where I'll also make that change to have a separate page for each valuecurve

No, haven't rebuilt the docs. Hopefully all the changes here should be independent of your efforts over there, should be minimal merge conflicts.

@jd-lara jd-lara merged commit 15d63de into psy4 Jun 21, 2024
1 of 8 checks passed
@jd-lara jd-lara deleted the gks/cost_alias_getters branch July 1, 2024 23:53
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