lutraconsulting / MDAL

Mesh Data Abstraction Library
http://www.mdal.xyz/
MIT License
160 stars 50 forks source link

Changed default for ASCII_DAT time TIMEUNITS between QGIS 3.8 and QGIS 3.10.1 #182

Closed saberraz closed 4 years ago

saberraz commented 4 years ago

Latest MDAL parses time for hydro_as-2d incorrectly. The upper image is the latest MDAL and the one in the bottom is the correct time (MDAL shipped with QGIS 3.8).

image002 (1) unnamed

PeterPetrik commented 4 years ago

@saberraz missing one of the two pictures.

saberraz commented 4 years ago

updated the OP.

PeterPetrik commented 4 years ago

This one is tricky, let me explain the situation

| QGIS | MDAL | TIMEUNITS | Default | link 

|---------|--------|------|-----------|
| 2.x     |  N/A   | No   | Seconds   
| 3.8.    | 0.3.3  | No   | Seconds   
| 3.10.0  | 0.3.3  | No   | Seconds
| 3.10.0  | 0.4.0  | Yes  | Hours
| 3.12.x  | 0.5.x  | Yes  | ?

old crayfish implementation https://github.com/lutraconsulting/qgis-crayfish-plugin/blob/7ad50a456dfb5d9340cf0dd60bbfdfe9b6d0eb2c/corelib/frmts/crayfish_dataset_dat.cpp#L513

PR that changed default: https://github.com/lutraconsulting/MDAL/commit/bf638c19c92a1e0dea91f6b942dc97b84352ee54#diff-6a5b644e181ca35770ef3dbeea8d4dc2L323

TIMEUNITS support: https://github.com/lutraconsulting/MDAL/blob/1aa79d395c4f107eb1586010d72c3fd4dd9f18d3/mdal/frmts/mdal_ascii_dat.cpp#L455

This issue is that ASCII_DAT is used by BASEMENT, HYDRO_AS-2D and TUFLOW solvers, with each different default HYDRO_AS-2D: Seconds TUFLOW: Hours

Standard does not define the default units at all, see https://www.xmswiki.com/wiki/SMS:ASCII_Dataset_Files_*.dat , but recommends to use TIMEUNITS flag to specify it. So either seconds or hours as default is compliant with standard.

So the clean solution is to add this line at the line before definition of first timestep data (TS flag)

TIMEUNITS Seconds 
TIMEUNITS Hours

In QGIS 3.12 hopefully we will have https://github.com/qgis/QGIS/issues/33196 implemented so user can tweak such problems

PeterPetrik commented 4 years ago

@saberraz @wonder-sk any suggestion if close this as "won't fix" (default hours) or get back to seconds default?

BenediktRotheOnGITHub commented 4 years ago

This is Benedikt Rothe from Hydrotec - the developers of HYDRO_AS-2D. We will add "TIMEUNITS Seconds" to dat-files in our next major-release.

BenediktRotheOnGITHub commented 4 years ago

We added TIMEUNITS to the dat-files in HYDRO_AS-2d. Unfortunately MDAL (QGIS 3.10) does not evaluate this, since the datasets are the "old format" (MDAL::DriverAsciiDat::loadOldFormat).

We'd prefere not to switch to the "new format", since lot's of customers have code to read these files and we don't want to break there code.

Would Lutraconsult evaluate timeunits for old format too?

Regards, Benedikt

PeterPetrik commented 4 years ago

@BenesName it should be possible to implement, created issue https://github.com/lutraconsulting/MDAL/issues/196

PeterPetrik commented 4 years ago

released 0.4.1 for QGIS 3.10.2 that should keep the default to hours. But also it adds option to specify TIMEUNITS manually for both old&new formats. So to change your dataset, manually add TIMEUNITS to the input format. https://github.com/qgis/QGIS/pull/33687

From QGIS 3.12+, the QGIS GUI has option to override the TIMEUNITS (in case or incorrectly parsed by MDAL) Screenshot 2020-01-10 at 11 28 38