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

Improve TIMModel implementation by updating the timeseries data type. #511

Closed
priscavdsluis opened this issue Apr 5, 2023 · 3 comments · Fixed by #521
Closed

Improve TIMModel implementation by updating the timeseries data type. #511

priscavdsluis opened this issue Apr 5, 2023 · 3 comments · Fixed by #521
Assignees
Milestone

Comments

@priscavdsluis
Copy link
Contributor

priscavdsluis commented Apr 5, 2023

What is the need for this task.
In issue #348 support of the tim file format was introduced.
However the timeseries attribute on the TimModel has a dictionary as type.
This was decided because the times in the time series data should be unique and with each a time a set of values is associated.

But the PolyFile, XYNModel, XYZModel and ForcingBase all do it differently than the tim file.

What is the task?

  • Discuss with @veenstrajelmer and team which type is suited
  • Update the type of the TimModel.timeseries to e.g. List[TimRecord] where TimRecord has two attributes: time (float) and data (List[float])
  • Add validation for unique times in TimModel.
@priscavdsluis priscavdsluis added this to To do in HYDROLIB-core via automation Apr 5, 2023
@priscavdsluis priscavdsluis added this to the Release 0.5 milestone Apr 5, 2023
@priscavdsluis
Copy link
Contributor Author

I have not made this a feature/bug because it is a loose tail from #348

@priscavdsluis priscavdsluis moved this from To do to Reviewer follow up in HYDROLIB-core Apr 5, 2023
@priscavdsluis priscavdsluis moved this from Reviewer follow up to To do in HYDROLIB-core Apr 5, 2023
@veenstrajelmer
Copy link
Collaborator

Copied from https://github.com/Deltares/dfm_tools/blob/main/tests/examples_workinprogress/workinprogress_meshkernel_creategrid.py

While doing a quick test I noticed the TimModel is not quite in line with other similar hydrolib-core objects. Since the timfile is similar than the files for XYNModel/XYZModel etc, I would expect more similarities in the object formatting. Below is some example code that shows how the objects are built up. In line with this, I would expect TimModel to have a points property and TimModel.points to be a list of Points (or similar object). Not set in stone of course, but it is an expectation which can be discussed.

For completeness, the Timeseries object (from a bc-file) is also included in the example code, but since that object is way more complex it might not be relevant.

import hydrolib.core.dflowfm as hcdfm

#xyz model
file_xyz = r'p:\archivedprojects\11205258-006-kpp2020_rmm-g6\C_Work\08_RMM_FMmodel\geometry_j19_NL_6-v2\rmm_vzm_v1p1_initial_water_level_onlyriv.xyz'
data_xyz = hcdfm.XYZModel(file_xyz)
print('\nXYZModel.points is list of XYZPoint, which has x/y/z/comment keys (first 5 are printed):') #TODO: comment can be dropped?
print(type(data_xyz.points))
print(data_xyz.points[:5])

#load xyn model
file_xyn = r'p:\archivedprojects\11206813-006-kpp2021_rmm-2d\C_Work\31_RMM_FMmodel\geometry_j19_6-v2\output_locations\rmm_vzm-j19_6-v2b_3_measurement_obs.xyn'
data_xyn = hcdfm.XYNModel(file_xyn)
print('\nXYNModel.points is list of XYNPoint, which has x/y/n keys (first 5 are printed):')
print(type(data_xyn.points))
print(data_xyn.points[:5])

#load pol/tek/pli/ldb file
file_pli = r'p:\11208054-004-dcsm-fm\models\model_input\bnd_cond\pli\DCSM-FM_OB_all_20181108.pli'
polyfile_object = hcdfm.PolyFile(file_pli)
poly_object_first = polyfile_object.objects[0]
print('\nPolyObject.points is list of Point, which has x/y/z/data keys (first 5 are printed):')
print(type(poly_object_first.points))
print(poly_object_first.points[:5])

# #bcfile
# file_bc_ts = r'c:\DATA\dfm_tools_testdata\hydrolib_bc\DCSM\steric_DCSM-FM_OB_all_20181108_firstpart.bc'
# m = hcdfm.ForcingModel(file_bc_ts)
# timeseries_object = m.forcing[0]
# print('\nTimeseries.datablock is list of lists with [time, val] (first 5 are printed):')
# print(type(timeseries_object.datablock))
# print(timeseries_object.datablock[:5])

#timfile
file_tim = r'p:\archivedprojects\11205258-006-kpp2020_rmm-g6\C_Work\08_RMM_FMmodel\structures_toRTC\Algerakering_GateLowerEdgeLevel.tim'
data_tim = hcdfm.TimModel(file_tim)
print('\nTimModel.timeseries is dict with times as keys and rest of columns as list of values:')
print(type(data_tim.timeseries))
print(data_tim.timeseries)

@veenstrajelmer
Copy link
Collaborator

Alternatively, we could consider to add the refdate from the mdu to the timfile (as a property) if it is being read via the FMModel tree

@MRVermeulenDeltares MRVermeulenDeltares moved this from To do to In progress in HYDROLIB-core Apr 11, 2023
@MRVermeulenDeltares MRVermeulenDeltares self-assigned this Apr 12, 2023
MRVermeulenDeltares added a commit that referenced this issue Apr 12, 2023
@MRVermeulenDeltares MRVermeulenDeltares moved this from In progress to Ready to review in HYDROLIB-core Apr 13, 2023
@tim-vd-aardweg tim-vd-aardweg moved this from Ready to review to Review in progress in HYDROLIB-core Apr 14, 2023
@tim-vd-aardweg tim-vd-aardweg moved this from Review in progress to Review/test follow up in HYDROLIB-core Apr 14, 2023
@MRVermeulenDeltares MRVermeulenDeltares moved this from Review/test follow up to In progress in HYDROLIB-core Apr 14, 2023
MRVermeulenDeltares added a commit that referenced this issue Apr 14, 2023
@MRVermeulenDeltares MRVermeulenDeltares moved this from In progress to Ready to review in HYDROLIB-core Apr 14, 2023
@tim-vd-aardweg tim-vd-aardweg moved this from Ready to review to Review in progress in HYDROLIB-core Apr 14, 2023
@tim-vd-aardweg tim-vd-aardweg self-assigned this Apr 14, 2023
MRVermeulenDeltares added a commit that referenced this issue Apr 14, 2023
@tim-vd-aardweg tim-vd-aardweg moved this from Review in progress to Test in HYDROLIB-core Apr 14, 2023
@veenstrajelmer veenstrajelmer moved this from Test to Review/test follow up in HYDROLIB-core Apr 14, 2023
@veenstrajelmer veenstrajelmer moved this from Review/test follow up to Review in progress in HYDROLIB-core Apr 14, 2023
@veenstrajelmer veenstrajelmer moved this from Review in progress to Ready to merge in HYDROLIB-core Apr 14, 2023
tim-vd-aardweg pushed a commit that referenced this issue Apr 17, 2023
HYDROLIB-core automation moved this from Ready to merge to Done Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
HYDROLIB-core
  
Done
4 participants