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

Change plural form of column names to singular #102

Open
glatterf42 opened this issue Jul 11, 2024 · 3 comments
Open

Change plural form of column names to singular #102

glatterf42 opened this issue Jul 11, 2024 · 3 comments
Labels
enhancement New feature or request

Comments

@glatterf42
Copy link
Member

          Is this on purpose that these are plural (values, units)?

Originally posted by @danielhuppmann in #101 (comment)

As mentioned above, many optimization objects at the moment expect to retrieve hardcoded column names such as elements, values, and levels, as appropriate. These names should be changed to their singular form in the DB layer (please correct me if this is wrong, @danielhuppmann). This could happen before the respective PRs are merged or with a separate cleanup immediately afterwards.

@glatterf42 glatterf42 self-assigned this Jul 11, 2024
@glatterf42 glatterf42 added the enhancement New feature or request label Jul 11, 2024
@glatterf42 glatterf42 removed their assignment Jul 11, 2024
@danielhuppmann
Copy link
Member

danielhuppmann commented Jul 11, 2024

Thanks for creating this issue @glatterf42. I looked a bit more closely at the previous ixmp API vs. the new implementation, and I think the changes are not that big...

The main issue...

The methods ixmp.Scenario.add_par(name, data) takes and the ixmp.Scenario.par(name) returns a dataframe with columns value and unit (and variable/equation with columns level and marginal).

I assume that many users have elaborate scripts for working with these dataframe in pre- or postprocessing, so in order to keep disruption during migration to ixmp4 minimal, we have to maintain this behavior with the methods ixmp4.Run.optimization.Parameter.add() and ixmp4.Run.optimization.Parameter.data.

Methods that were introduced only in ixmp4 like ixmp4.Run.optimization.IndexSet.elements or ixmp4.Run.optimization.Parameter.units can remain in plural form or be converted to singular, not sure which is more intuitive...

Not sure which strategy is better, adapting the PRs or doing a follow-up PR...

@glatterf42
Copy link
Member Author

In principle, this would probably be something that the shim layer that @khaeru suggested could take care of. However, we can of course adjust the names of the columns in the dataframes for historical consistency.
I would prefer to do this as a clean-up PR, I think, because it was a bit tedious to cherry-pick all commits for #101 since they all required very similar resolutions of merge conflicts and I would like to avoid repeating this. No one from the team is likely to just suddenly explore the tutorial, either, so I would start a clean up PR as soon as the data-model-PRs and the tutorial is merged and advertise only once that is complete. This PR would

  • rename the columns in the returned dataframes
  • adjust the tutorial and potentially
  • rename the DB-table columns as well for consistency

For things like Indexset.elements, I would intuitively opt to keep the plural form because if I run Indexset.element, I would not expect to receive a list.

@danielhuppmann
Copy link
Member

Sounds good to me to do the consistency-renaming as a follow-up PR, and agree to keep the elements as plural.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants