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

Make robust #3

Merged
merged 37 commits into from
Jul 25, 2024
Merged

Make robust #3

merged 37 commits into from
Jul 25, 2024

Conversation

fabern
Copy link
Member

@fabern fabern commented Jul 23, 2024

This PR makes map2tidy more robust (solves #2):

  • consistent return value (nested data.frame if time present, non-nested otherwise) regardless of input options (e.g. do_chunks, ncores)
  • handling of multiple calendars (e.g. julian, 365_day (a.k.a noleap), 366_day, 360_day)
  • Increase verbosity
  • Streamline code (apply DRY principle)
  • Store RDS with lon-value in filename (instead of lon-iterator)

Handling of calendars was achieved by returning strings in the 'datetime' column of the tidy data.frames. Parsing of these strings, if necessary, must be done afterwards by the user. Then days such as "2021-02-30" (valid in a 360_day-calendar), can be handled accordingly.

@stineb
Copy link
Collaborator

stineb commented Jul 23, 2024

Wonderful. Thanks. Some checks are not passing. That's why I haven't merged just yet.

@fabern
Copy link
Member Author

fabern commented Jul 23, 2024

Yes, this is still in the draft state.
I'm finishing the last TODO item to rename the RDS files now.

The checks are failing because I require the development version of tidync. The workaround for the older is already in place. It just has to be activated as default.

@fabern fabern marked this pull request as ready for review July 24, 2024 07:39
@fabern
Copy link
Member Author

fabern commented Jul 24, 2024

@stineb Should we increase version number to 2.0.0?
API changes are incompatible and require a MAJOR increment.

@fabern fabern mentioned this pull request Jul 24, 2024
3 tasks
@stineb
Copy link
Collaborator

stineb commented Jul 24, 2024

Yes, calling this v2.0 makes sens. However, we haven't tagged anything yet and haven't created releases. Let me know before you create a release so that I can sync it to my Zenodo.

@stineb
Copy link
Collaborator

stineb commented Jul 24, 2024

Is there a reason for changing from varnam to varnames? I used varnam because I call it that way in some other functions that are now in {rgeco}.

@fabern
Copy link
Member Author

fabern commented Jul 24, 2024

The reason I changed varnam to varnames was to make more evident that it can be a vector of strings.
Since I was changing anyway the API I felt this clarifies the use cases.
Should I roll back to using varnam?

@stineb stineb merged commit e53719b into main Jul 25, 2024
7 checks passed
@fabern fabern deleted the make-robust branch July 30, 2024 13: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.

2 participants