Bringing in snowing by bizzinho · Pull Request #43 · SPL-ethz/snow

bizzinho · 2023-07-04T11:10:01Z

No description provided.

Model for spatial evolution within a single vial. Developed by Andraz Kosir, as part of his M.Sc. thesis project.

Revised language of new part on v 2.0

Also updated the general part at the beginning

Added description for V2.0

Added Andraz

updated pallet article from pre-print to final version

updated for v2.0

updated for version 2

bizzinho

Review is still WIP. In particular, snowing.py, utils.py, and single_vial_spatial_model.py have not been reviewed yet. Still, I think the first comments can be of use / allow you to get started.

The main comment are

there are no new unit tests, which is a gap.
the purpose of sample.py is unclear
I am not convinced that this has been "regression tested", i.e., made sure that the existing functionalities work as expected by the user/explained in the tutorial

bizzinho · 2023-07-04T11:11:17Z

src/ethz_snow/config/snowConfig_default.yaml

-    shape: cube
-    # length [m]
+    # base shape of vial (in the spatial version only cylinder is accepted)
+    shape: cylinder


Won't this change to the default break snow and snowfall?

It at least breaks the associated unit test

FAILED tests/test_constants.py::test_warningWrongYamlSchema - assert 7.853981633974483e-07 == ((1 * 0.01) * 0.01)

This is due to the fact that the expectation was that the default shape is a cube

We cannot really generalize the shape for all models, so my idea was that we keep cube as the default shape for snow and snowfall, but we change it to cylinder for snowing. Then the users have to change this depending on which model they want to use.

bizzinho · 2023-07-04T11:13:55Z

sample.py

What is the purpose of this file? Can this be integrated into the tutorial?

I guess it could be integrated into the tutorial. So far it was actually just for me for quickly running different model dimensionalities while testing the changes. Forgot to remove it

bizzinho · 2023-07-04T11:17:47Z

src/ethz_snow/config/snowConfig_default.yaml

 solution:
-  # kg/m^3 density of water / assumed constant for all phases
+  # solute mass fraction [-]
+  solid_fraction: 0.25


how does this change/increase affect snow and snowfall?

Sorry, default should stay 0.05. I changed it back.

bizzinho · 2023-07-04T11:18:43Z

src/ethz_snow/config/snowConfig_default.yaml

-  # °C equilibrium freezing temperature pure water
-  T_eq: 0
-  # initial temperature of the solution
-  T_init: 20


you have removed T_init without replacement, does this not break snow and snowfall?

If I checked everything, T_init is not used anywhere since the pallet update (it's commented out in snowflake

bizzinho · 2023-07-04T11:22:51Z

src/ethz_snow/constants.py


 from typing import Optional, List, Any

-from .__init__ import __citation__, __bibtex__


please don't remove stuff unless you are sure that this is what you want :)

Don't know what happened here, accidentally I guess, I put it back now.

bizzinho · 2023-07-04T11:27:15Z

src/ethz_snow/config/snowConfig_default.yaml

@@ -1,36 +1,87 @@
+# temperature resolution within vial (homogeneous, spatial_1D, spatial_2D)
+temperature: spatial_2D


I feel that this is not a informative name for this key. How about temp_model?

or temp_dim or dimensionality

Fully agree, changed to dimensionality.

bizzinho · 2023-07-04T11:46:06Z

src/ethz_snow/constants.py

+                )
+            )
+
+    # set up additional parameters for VISF


I assume this comment should read "jacket", not VISF?

Correct, changed.

bizzinho · 2023-07-04T11:46:29Z

src/ethz_snow/constants.py

+    constVars.extend(["lambda_air", "air_gap"])
+
+    if config["temperature"] != "homogeneous":
+        if config["vial"]["geometry"]["shape"] != "cylinder":


This should be unit tested

bizzinho · 2023-07-04T11:46:57Z

src/ethz_snow/constants.py

+
+    elif config["vial"]["geometry"]["shape"].startswith("cyl"):
+        geoKeys = config["vial"]["geometry"].keys()
+        if ("radius" in geoKeys) and (config["vial"]["geometry"]["radius"] is not None):


bizzinho · 2023-07-04T11:50:06Z

src/ethz_snow/constants.py

+        k_f = float(config["solution"]["k_f"])
+        M_s = float(config["solution"]["M_s"])
+
+        # heat conductivity


confusing/contradicting comments above and below line

44 fix pipeline

bizzinho

Continuing my review.. I believe there are now quite a few comments. Maybe I make a pause here and let you react before I continue with the meat of snowing

bizzinho · 2023-07-04T14:52:19Z

src/ethz_snow/constants.py

+            config["vial"]["geometry"]["width"]
+        )
+
+    elif config["vial"]["geometry"]["shape"].startswith("cyl"):


Correct me if I'm wrong, but if the vial geometry is cylinder, then it has to be a single vial simulation, right? There is no implementation of the intervial heat fluxes for cylinders, is there?

If I'm right, there should be a check for this here.

Exactly, if shape is cylinder then it is only a single vial spatial simulation. If cube, then it cannot be a spatial model as it is right now.

bizzinho · 2023-07-04T14:53:47Z

src/ethz_snow/single_vial_spatial_model.py

What is the purpose of this file?

This file is not used in the package, was just used to make a piece of code, which generates a few plots used in a conference abstract public. Should be removed.

bizzinho · 2023-07-04T14:55:34Z

src/ethz_snow/utils.py

+
+
+# vapour pressure estimation (temperature in K, pressure in Pa)
+def vapour_pressure_liquid(T_liq):


Would be great to have a source or ref for this state eq

bizzinho · 2023-07-04T14:55:45Z

src/ethz_snow/utils.py

+
+
+# vapour pressure estimation (temperature in K, pressure in Pa)
+def vapour_pressure_solid(T_sol):


Same as above

bizzinho · 2023-07-04T14:58:41Z

src/ethz_snow/utils.py

+
+# vapour pressure estimation (temperature in K, pressure in Pa)
+def vapour_pressure_liquid(T_liq):
+    """_summary_


I added a docstring skeleton, please fill it out @akosira

Filled out.

bizzinho · 2023-07-04T15:11:31Z

src/ethz_snow/snowing.py

+            raise NotImplementedError(
+                (
+                    f'Configuration "{self.configuration}" '
+                    + "not correctly specified, use shelf, jacket or VISF."


check in constants

Shouldn't it be checked here as well, if II allow passing configuration and model dimensionality as an inputs directly?

Actually, I now removed configuration, dimensionality as additional parameters to snowing. Everything is specified in configPath now. Only check is in constants.

bizzinho · 2023-07-04T15:17:36Z

src/ethz_snow/snowing.py

+        k: dict = {"int": 0, "ext": 0, "s0": 50, "s_sigma_rel": 0},
+        opcond: OperatingConditions = OperatingConditions(),
+        temperature: str = "spatial_1D",
+        configuration: str = "shelf",


I'm not yet convinced it makes sense to be able to provide configPath and temperature and configuration.

The idea of keeping temperature and configuration inputs was that once all the relevant constants are specified in the yaml file, the user could run the model for different configurations and thus with different dimensionalities without having to again modify the yaml file constantly. what do you think?

I now removed configuration, dimensionality as additional parameters to snowing. Everything is specified in configPath now. I think you were right, this makes much more sense!

bizzinho · 2023-07-04T15:19:22Z

src/ethz_snow/snowing.py

+        self._prop = None
+        self._time = None
+        self._shelf = None
+        self._temp = None


For consistency, it would be nice, albeit not critical, to use the same logic with a state variable X that includes both temp and ice, then make temp and ice properties that are derived from X

I see, but I would leave it like this actually. I find it more clear

bizzinho · 2023-07-04T15:23:36Z

src/ethz_snow/snowing.py

+        return self._simulationStatus
+
+    @property
+    def getTime(self) -> np.ndarray:


Here, getTime is a property and the @property decorator defines this to be a getter function. A more natural name for this variable would be "time"

The same holds true for all the other get... properties, of course

Makes sense, changed.

bizzinho · 2023-07-04T15:26:56Z

src/ethz_snow/snowing.py

+            return self._time
+
+        else:
+            raise AssertionError("Simulation not yet carried out or completed.")


If you really want to have this be an assertion error (which I think is fine), it's probably easier to just make the if condition an assert statement instead...

assert False, "This is False"

Makes sense, thanks. Adapted

Docstrings annotated

…r, warning is printed

Added spatial model article DOI

…to snowing-develop

ltdeck and others added 14 commits June 26, 2022 15:03

Initial single vial commit

8d9e591

Model for spatial evolution within a single vial. Developed by Andraz Kosir, as part of his M.Sc. thesis project.

Snowing added, tutorial updated

fda2217

tutorial updated for Snowing

909e155

Update tutorial.rst

df81078

fixing typo in tutorial.rst

2e13a1b

another typo in tutorial.rst

bd03350

Update tutorial.rst

7480ff2

Revised language of new part on v 2.0

Update tutorial.rst

639c112

Also updated the general part at the beginning

Update tutorial.rst

2f72061

Update changelog.rst

5c31fe1

Added description for V2.0

Update authors.rst

1d22b0a

Added Andraz

Update refs.bib

30895b0

updated pallet article from pre-print to final version

Update README.md

3301078

updated for v2.0

Update LICENSE.txt

9a5e1ee

updated for version 2

bizzinho assigned akosira Jul 4, 2023

bizzinho added the enhancement New feature or request label Jul 4, 2023

minor rearrangements in constants

7845ec7

bizzinho commented Jul 4, 2023

View reviewed changes

bizzinho and others added 7 commits July 4, 2023 14:33

remove use of deprecated append

393deea

Remove use of deprecated iteritems

448c27b

Merge pull request #45 from SPL-ethz/44-fix-pipeline

17dc4fa

44 fix pipeline

Merge branch 'develop' into snowing-develop

0b651ca

Merge branch 'develop' into snowing-develop

11ba252

docstring skeletons

31d2fa7

Linting and minor rearrangements

f042446

bizzinho commented Jul 4, 2023

View reviewed changes

bizzinho requested a review from ltdeck July 4, 2023 15:28

bizzinho marked this pull request as draft July 5, 2023 10:09

akosira added 2 commits July 6, 2023 14:19

Changed key name temperature to dimensionality

2ba1164

Update utils.py

221f13e

Docstrings annotated

akosira added 25 commits July 6, 2023 15:42

Update snowing examples

819ca06

added repr function to snowing

7f21154

Default shape changed back to cube

244c2df

Updated comments in constants.py

64137cb

Updated constants.py, for spatial model shape is rewritten to cylinde…

bf6f8ce

…r, warning is printed

Update constants.py

a1dec07

Added test_snowing.py, but need to add a lot more tests

42033ba

Updated constants.py, moved checks from snowing

4a2a017

Removed single_vial_spatial_model and added more snowing tests

08cb460

more snowing tests added to improve coverage

32f1e03

improved coverage

9c79af1

Update tutorial.rst

4ab730f

modified getter functions snowing

2a65144

a few more tests for snowing plots

3971984

one more plot test

c99c97d

and one more test

44d93c3

Update tutorial.rst

d119abd

Update README.md - added SNOWing doi to description

1618688

scalar mappable bug fix

aeee07e

Update CITATION

d862408

Added spatial model article DOI

final touches

cefb0c2

Merge branch 'snowing-develop' of https://github.com/SPL-ethz/snow in…

6891ac9

…to snowing-develop

more final touches

36c5bc2

final touches more more more

637c5d8

some new docs files

4f85310


		from typing import Optional, List, Any

		from .__init__ import __citation__, __bibtex__

		@@ -1,36 +1,87 @@
		# temperature resolution within vial (homogeneous, spatial_1D, spatial_2D)
		temperature: spatial_2D



		# vapour pressure estimation (temperature in K, pressure in Pa)
		def vapour_pressure_liquid(T_liq):



		# vapour pressure estimation (temperature in K, pressure in Pa)
		def vapour_pressure_solid(T_sol):

Conversation

bizzinho commented Jul 4, 2023

Uh oh!

bizzinho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bizzinho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment