nens / threedi-api-qgis-client

3Di Models & Simulations plugin
https://plugins.lizard.net/
Other
2 stars 0 forks source link

Simulation wizard: Improve Laterals page #467

Closed leendertvanwolfswinkel closed 1 month ago

leendertvanwolfswinkel commented 1 year ago

The current UI does not correspond with the possible use cases for laterals. Proposed UI:

image.png

leendertvanwolfswinkel commented 7 months ago

@leendertvanwolfswinkel Check if units in spatialite are converted from minutes to seconds somehow? There was a ticket for this (weird vague thing)

hoanphungt commented 3 months ago

@leendertvanwolfswinkel is it possible to select both 1d laterals and 2d laterals or only one of them is allowed? Previously, there is a dropdown to select 1d or 2d laterals so I thought only one of them can be selected at one time.

leendertvanwolfswinkel commented 3 months ago

@hoanphungt this is one of the main reasons to re-implement this page. It is a perfectly valid option to have both 1D and 2D laterals in your simulation, but the current UI implementation won't allow you to do this

hoanphungt commented 3 months ago

For this bullet:

  • The whole group "1D laterals" should be disabled if there is no 1D in the threedimodel

How do I know if there is no 1D / 2D in the 3di model?

Answer: if the 3di model contains extent_one_D or extent_two_D

And similarly for this bullet:

  • Use 1D laterals from simulation template" should be disabled if there are no 1D laterals in the simulation template

How do I know if there is 1D / 2D laterals in the simulation template?

Answer: if there is "point" key in the laterals => 2D else it's 1D

hoanphungt commented 2 months ago

@leendertvanwolfswinkel I am going pretty far with this laterals page. There are still a few more questions for you though:

  1. the use 1d / 2d laterals from simulation template option: does it inherit the events.laterals from the simulation template or events.filelaterals? This option is not available in the old laterals page.

  2. Can you elaborate more on the Add to existing and Replace existing options for 1D laterals? By default, which option should be selected first or are they both optional?

leendertvanwolfswinkel commented 2 months ago

@hoanphungt 1: if the user says "from template", it should be exactly the same as the template. So both the events.laterals and the events.filelaterals. 2: Let's make our lives a bit simpler and remove this choice. The behaviour should just be that if the user checks both the "Use laterals from simulation template" and "Upload laterals" option, the uploaded laterals are added to the ones from the simulation template. You can just post to the same endpoint multple times for laterals, iirc.

leendertvanwolfswinkel commented 2 months ago

@hoanphungt I tested it and have the following feedback:

TypeError: 'str' object does not support item assignment 
Traceback (most recent call last):
  File "C:\Users\leendert.vanwolfswin\AppData\Roaming\3Di\QGIS3\profiles\default/python/plugins\threedi_models_and_simulations\widgets\simulation_wizard.py", line 635, in interpolate_changed
    val["interpolate"] = interpolate
TypeError: 'str' object does not support item assignment

laterals 2D with type and XY (AS_XY) geometry in WGS84 poging 3.csv

laterals 1d.csv

They are accepted by the validation that occurs directly after selecting the file. However, when I try to start the simulation, I get this error:

NotImplementedError 
Traceback (most recent call last):
  File "C:\Users\leendert.vanwolfswin\AppData\Roaming\3Di\QGIS3\profiles\default/python/plugins\threedi_models_and_simulations\widgets\simulation_wizard.py", line 2972, in run_new_simulation
    constant_laterals, file_laterals = self.laterals_page.main_widget.get_laterals_data(
  File "C:\Users\leendert.vanwolfswin\AppData\Roaming\3Di\QGIS3\profiles\default/python/plugins\threedi_models_and_simulations\widgets\simulation_wizard.py", line 689, in get_laterals_data
    file_laterals.update(self.recalculate_laterals_timeseries(self.TYPE_1D, timesteps_in_seconds))
  File "C:\Users\leendert.vanwolfswin\AppData\Roaming\3Di\QGIS3\profiles\default/python/plugins\threedi_models_and_simulations\widgets\simulation_wizard.py", line 667, in recalculate_laterals_timeseries
    raise NotImplementedError
NotImplementedError
hoanphungt commented 2 months ago

@leendertvanwolfswinkel I fixed both errors. Could you please try again?

hoanphungt commented 2 months ago

The 2d laterals file should have columns in the following order:

X Y type id timeseries

hoanphungt commented 2 months ago

@leendertvanwolfswinkel this issue can be tested again on the master branch

leendertvanwolfswinkel commented 1 month ago

@hoanphungt I have tested again. I am now able to upload both 1D and 2D laterals in the same simulation.

However, my input is:

What the API says it has received:

So the 1D laterals with IDs 1, 2, and 3 are missing.

Model: Akersloot (Leendert) #3 1D laterals CSV: laterals_1d.csv

2D laterals CSV: laterals_2d_x_y_id_timeseries_wgs84.csv

leendertvanwolfswinkel commented 1 month ago

@hoanphungt one more minor design issue. I noticed that the "1D laterals" header looks different (I think just smaller font size) than the 2D laterals header. Please make them both the same as the 2D laterals header

image

hoanphungt commented 1 month ago

@hoanphungt I have tested again. I am now able to upload both 1D and 2D laterals in the same simulation.

However, my input is:

  • 3 2D laterals (IDs 1, 2, 3)
  • 4 1D laterals (IDs 1, 2, 3, 4)

What the API says it has received:

  • 3 2D laterals (ID 1, 2, 3)
  • 1 1D lateral (ID 4)

So the 1D laterals with IDs 1, 2, and 3 are missing.

Model: Akersloot (Leendert) #3 1D laterals CSV: laterals_1d.csv

2D laterals CSV: laterals_2d_x_y_id_timeseries_wgs84.csv

@leendertvanwolfswinkel It seems that laterals 1d and 2d can have the same id indeed. I didn't know about it. Can you test again in this PR: https://github.com/nens/threedi-api-qgis-client/pull/555?

leendertvanwolfswinkel commented 1 month ago

@hoanphungt tested it again and it works as expected. Please also fix the minor design issue mentioned in my previous comment and merge to master

hoanphungt commented 1 month ago

@leendertvanwolfswinkel I don't have the minor design issue that you mentioned so that's strange.