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

Possibly disable month and year for Duration parameter type #321

Open
jkiviluo opened this issue Nov 27, 2023 · 16 comments
Open

Possibly disable month and year for Duration parameter type #321

jkiviluo opened this issue Nov 27, 2023 · 16 comments
Labels
Milestone

Comments

@jkiviluo
Copy link
Member

jkiviluo commented Nov 27, 2023

While Python, Julia and SpineOpt all handle month and year correctly for Duration parameter (in Python using dateutils.relativedelta), there are some drawbacks:

  • months and years have changing duration --> most models don't know how to deal with those (user could use them and get weird behaviour)
  • Python's dateutils.relativedelta (and probably Julia's equivalent) is slower than datetime.timedelta, since the latter does not deal with complex if clauses

Our options:

  • keep doing as now (use relativedelta, leave it to user not to srew-up), but:
    • improve documentation
    • add a warning note where appropriate
  • implement timedelta as the default parameter type for Duration (leave relativedelta as an option)
    • speed improvement should be tested
  • switch completely to timedelta
    • check if anyone is using months or years as Duration in their models - and are these important use cases where the workaround is non-satisfactory

Relates to: #319

@jkiviluo jkiviluo added this to the v1.0 milestone Nov 27, 2023
@jkiviluo jkiviluo added the speed label Nov 27, 2023
@DillonJ
Copy link

DillonJ commented Nov 27, 2023

I know this is not the intention here but just wanted to make sure that this wouldn't impact time pattern syntax where it should still be ok to use months and years which we are doing now quite extensively

@manuelma
Copy link
Collaborator

My preference here would be to add a clear note so users know what they're messing with - but don't disable the support. I am not sure Julia is affected by this since they don't provide any simpler API. The only API does things right so probably it's well optimised.

@jkiviluo
Copy link
Member Author

Please note: this is about resolution parameter. We would still allow to set parameter values for e.g. a particular month. Setting parameters for a particular month doesn't have the same issue, since then one is talking about a particular month that has a particular number of days - instead of duration of abstract month where the number of days is not known.

@manuelma
Copy link
Collaborator

I think to be fully clear we should say that this is about the resolution property for the TimeSeriesFixedResolution.

@jkiviluo I should also point out that if the user gives the resolution, they also need to give the start, and that makes everything concrete.

@jkiviluo
Copy link
Member Author

Oh no, I've mixed things up again. I was really thinking about the Duration parameter type. For TimeSeriesFixedResolution things are indeed clear. I've clean up the issue name and description.

@jkiviluo jkiviluo changed the title Possibly disable month and year for resolution parameter types Possibly disable month and year for Duration parameter type Nov 27, 2023
@jkiviluo
Copy link
Member Author

Although, if you have a DB with a temporal resolution of one month, most models will not be able to handle it correctly unless it's implemented with TimeSeriesVariableResolution.

I guess it could be a DB level choice too. I'm now working on the generic DB and there I need to worry about these things. For SpineOpt DB 1 month resolution would be fine though.

@manuelma
Copy link
Collaborator

I think I misled you to use the word 'resolution' - my point was that we should try and use the terminology in the docs as much as possible (https://spine-database-api.readthedocs.io/en/0.8-dev/parameter_value_format.html)

Just curious @jkiviluo when you say most models you mean which ones in particular?

@manuelma
Copy link
Collaborator

@jkiviluo also, is the generic DB specified somewhere? How are we working on this?

@soininen
Copy link
Collaborator

soininen commented Nov 28, 2023

At least the Python implementation of TimeSeriesFixedResolution handles monthly and yearly resolution correctly so if you e.g. export the time stamps to .csv you get the dates you would expect.

@jkiviluo
Copy link
Member Author

Just curious @jkiviluo when you say most models you mean which ones in particular?

I didn't check, but most models don't even handle variable time resolution. And even models like Backbone and FlexTool that can have variable time resolution wouldn't function if one tried to say that they should roll forward a 1 month Duration. Or if you tried to set their time Resolution parameter to 1 month, it wouldn't work either - it could be made to work, but it would require quite a bit of coding since their modelling languages don't know how to interpret 1 month (interpreting 4 weeks is trivial in comparison).

@jkiviluo
Copy link
Member Author

At least the Python implementation of TimeSeriesFixedResolution handles monthly and yearly resolution correctly so if you e.g. export the time stamps to .csv you get the dates you would expect.

Right, doing a script to handle the time series on Python side would make things easier for the Resolution parameter (for models that can handle variable time resolution). However, Duration (e.g. roll forward) requires understanding of the date. While it's plausible to manage that also in Python, it can require quite a bit of rewriting on the model side depending on the particular implementation.

@jkiviluo
Copy link
Member Author

Is the generic DB specified somewhere? How are we working on this?

It's currently in WP5, Task 5.2 folder. I plan to make a repository copy (as a JSON) soon. The repo would then be the place to discuss too, but for now it's just directly to me.

@jkiviluo
Copy link
Member Author

To get back to the original issue: is anyone using month or year for the Duration parameter? If so, we shouldn't start changing that right now. If not, we could consider it.

Otherwise this issue is for the future when we have more time to check if there is something to be gained on the speed side through disabling. Naturally we can implement the first option as soon as anyone has time for that - they won't preclude later options and would be helpful for the user.

@manuelma
Copy link
Collaborator

Thank you @jkiviluo - that sounds good. I think it's good to have the option even if for some models is 'slow'. It's not like we're rolling models like crazy, I doubt it will ever become a bottleneck (the time to roll a model is typically a small percentage of the 'solving time').

Also, the user knows the start date (at least in SpineOpt, and I assume everywhere), so they can know exactly where the rolling will jump each time.

Last, we're talking about roll_forward (or equivalent) in particular, but duration can be used in general and we don't know exactly what the user has in mind - so this is another reason to keep it.

Anyways, I'm inclined to keep it.

@jkiviluo
Copy link
Member Author

the user knows the start date (at least in SpineOpt, and I assume everywhere)

That's not the case - many models use pseudo-dates or timestamps that are not dates (e.g. t00001).

It would be great if there is no speed issue in the solves (but I'd like to know for sure - one needs to process all the time series every time the model rolls and in some cases rolling can be very frequent - e.g. when trying to resolve what happens intra-day).

The speed issue could also manifest itself in e.g. visualizations. Plotting times series with relativedelta could be considerably slower than plotting with timedelta. And it's not just data that actually has months or years, but it would affect all time series data, since the code cannot know before checking whether it needs to run through the hoops to calculate the lengths of months or years.

@nnhjy
Copy link
Member

nnhjy commented Feb 5, 2024

There seems to be some overlap with this SpineOpt discussion #869

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

No branches or pull requests

5 participants