search

EnergySystemsModellingLab / MUSE_2.0

Welcome to the MUSE 2.0 repository
GNU General Public License v3.0
1 stars 0 forks source link

Add code to read time slices CSV files #89

Open alexdewar opened 4 days ago

alexdewar commented 4 days ago

Description

This PR adds support for reading in time slices CSV files (e.g. examples/simple/time_slices.csv) (#59), which look like this:

season,time_of_day,fraction
winter,night,0.072916667
winter,day,0.104166667
winter,peak,0.03125
winter,evening,0.041666667
peak,night,0.072916667
peak,day,0.104166667
...

(Note that not every combination of season and time_of_day need be present -- as discussed with @ahawkes.)

I've written a function (read_time_slices) which currently returns a Vec<TimeSlice> because that was the simplest solution. Going forward, it would probably be better to represent this as a HashMap to make it easier to work with (performance isn't really a consideration with so few elements). I think that can wait for another PR though.

I've added a couple of checks to validate the input data: one to check the values sum to 1 (see #69) and another to check that they're all in the range 0 to 1. @dalonsoa suggested elsewhere that we might want to put common validation routines in a separate module. I did consider doing that here, but, given that both the checks are effectively one-liners, I didn't think that made sense right now. When we have some more places in the code that need to do these things, I think then is when we should make a common all_values_sum_to_one function etc.

For now, I've assumed that it is possible that users may want to create a model without any time slices (in this case, I guess we'd just make one big time slice that covered the whole year). Is this right? @ahawkes?

I'm opening this as a draft PR, because I want to wait until #80 is merged before merging this. Once that's done, I'll update this code to actually get the path to the CSV file from the settings struct.

Update: I figured out why the code didn't work in the training session: I just needed to increase the tolerance for the check that all the values sum to one. It made me realise that in future we probably need some functional tests to check that the example model(s) load without error so I've opened an issue for it (#90).

Closes #59. Closes #69.

Type of change

Key checklist

Further checks

codecov[bot] commented 4 days ago

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 67.74%. Comparing base (e24ddce) to head (2b3674e).

Files Patch % Lines
src/simulation.rs 0.00% 5 Missing :warning:
src/main.rs 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #89 +/- ## ========================================== + Coverage 0.00% 67.74% +67.74% ========================================== Files 2 3 +1 Lines 7 31 +24 ========================================== + Hits 0 21 +21 - Misses 7 10 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

alexdewar commented 4 days ago

@dalonsoa @Ashmit8583 @abhinavgupta2004 This is ready for a review now.

alexdewar commented 4 days ago

PS -- there's no particular rush on this. I'm on leave tomorrow (Thursday) anyway. I'll be back in action on Friday.

Ashmit8583 commented 4 days ago

Sure Sir We will have a look at it

On Wed, 26 Jun 2024, 8:22 pm Alex Dewar, @.***> wrote:

PS -- there's no particular rush on this. I'm on leave tomorrow (Thursday) anyway. I'll be back in action on Friday.

— Reply to this email directly, view it on GitHub https://github.com/EnergySystemsModellingLab/MUSE_2.0/pull/89#issuecomment-2192468129, or unsubscribe https://github.com/notifications/unsubscribe-auth/BAWEAHZFKPTQBSPDXTQKPGTZJMIHJAVCNFSM6AAAAABJ55ZOQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJSGQ3DQMJSHE . You are receiving this because you were mentioned.Message ID: @.***>

ahawkes commented 4 days ago

Thanks @alexdewar - looks good - and in response to the question yes we should assume one time slice with year fraction equal to one if no time slice information is provided. This should be the case if no link to a time slice file is provided in the toml, or if the file which is linked contains nothing (these could be different warnings, but both result in one time slice, and the user is told about it). Maybe we need a log file for the input data read to record any warnings/errors that arise?

dalonsoa commented 4 days ago

If there's a timeslice file but it is empty, I'd raise an error directly as it makes no sense to have such an empty file and might point to a mistake from the user's side. Only if there's no timeslice file at all I would throw a warning and setup a single all-year,all-day,1.0 timeslice.

alexdewar commented 3 days ago

Thanks @alexdewar - looks good - and in response to the question yes we should assume one time slice with year fraction equal to one if no time slice information is provided. This should be the case if no link to a time slice file is provided in the toml, or if the file which is linked contains nothing (these could be different warnings, but both result in one time slice, and the user is told about it).

Cool. I'll do that in this PR

Maybe we need a log file for the input data read to record any warnings/errors that arise?

Maybe. Whether or not we log to a file though, we definitely want to use a proper logging library so that we can have different log levels (and it would also include support for writing it to a file). I've opened an issue: #90