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

PYIODE: moved low_to_high() and high_to_low() in the Variables class #556

Merged
merged 3 commits into from
May 24, 2024

Conversation

alixdamman
Copy link
Collaborator

See PR #545

          * LTOH_LIN as LTOH_LINEAR
          * LTOH_CS as LTOH_CUBIC_SPLINESS
        - set LTOH_STOCK and LTOH_FLOW to 'S' and 'F' respectively
@alixdamman alixdamman merged commit 535ba45 into master May 24, 2024
8 checks passed
LTOH_LIN: str = 'L'
LTOH_CS: str = 'C'
LTOH_LINEAR: str = 'L'
LTOH_CUBIC_SPLINESS: str = 'C'
Copy link
Collaborator

Choose a reason for hiding this comment

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

The two S at the end of splines seem intended because you repeat them in other places but I don't understand why you use them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The two S at the end of splines seem intended

No. This is a misspelling. Good catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See PR #573

@@ -178,15 +178,15 @@ Below *workspace* must be replaced by either:
+----------------------+---------------------------------------------------------------------------------------------------------------------+
| ``WsExtrapolate`` | ``variables.extrapolate(method, from_, to_, variables_list)`` --> See :meth:`Variables.extrapolate` |
+----------------------+---------------------------------------------------------------------------------------------------------------------+
| ``WsLtohStock`` | ``low_to_high(LTOH_STOCK, method, filepath, var_list)`` --> See :func:`low_to_high` |
| ``WsLtohStock`` | ``variables.low_to_high(LTOH_STOCK, method, filepath, var_list)`` --> See :meth:`Variables.low_to_high` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are variables from the current workspace actually used in the process or are they completely overwritten or is is some kind of merge of variables from the file and the current workspace? In either case I think the two methods should be called either load_xxx or merge_xxx or something in that vein that indicates it does not convert the current variables.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will rewritte the docstring of low_to_high and high_to_low following the French documentation of IODE commands of WsLtoH(...) and WsHtoL(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See PR #575

@@ -909,6 +911,197 @@ cdef class Variables(_AbstractDatabase):
else:
return [cpp_period.decode() for cpp_period in self.database_ptr.get_list_periods(from_.encode(), to.encode())]

def low_to_high(self, type_of_series: str, method: str, filepath: str, var_list: Union[str, List[str]]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I still don't understand precisely the purpose of those two methods. Are these specifically meant to convert yearly to quarterly and vice versa, or can they work with any periodicities? If it's the former, I would rename the functions accordingly and document that behaviour, and if is the later, I don't understand how it works given that the user does not need to specify the target periodicity?!?

Copy link
Collaborator Author

@alixdamman alixdamman Jun 11, 2024

Choose a reason for hiding this comment

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

See PR #575

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.

2 participants