AguaClara / aguaclara

An open-source Python package for designing and performing research on AguaClara water treatment plants.
https://aguaclara.github.io/aguaclara/
MIT License
24 stars 13 forks source link

Test the new dictionary optional argument method #41

Closed fletchapin closed 6 years ago

fletchapin commented 6 years ago

In order to see how well the new method for storing constants in dictionaries and passing them in as optional arguments works we will start with the simplest file, lfom.py.

Once we're done modifying lfom.py we will test how well the new method works and if it's working well apply it to the other unit processes as well.

zoemaisel commented 6 years ago

We think that there should be a universal constants YAML (ex: ratio VC orifice, gravity, density of water). They should be put in their own template that is used in multiple places but we are not sure how that will work with the strip jinja function.

fletchapin commented 6 years ago

@ethan92429 How do we use units wrappers for dictionaries since that will be necessary for the optional argument?

Also, does aide_render parse units properly when multiple units are used. For example is "!u 1 m/s" parsed properly?

eak24 commented 6 years ago

check out the strict option in this

zoemaisel commented 6 years ago

@ethan92429 We have been using None for the unit wrappers when calling the dictionary. However, this means that we have to .magnitude every input. This also means that we are assuming that the dictionary value has a certain unit, unless we .to(unit) everything to ensure that the calculation is correct.

We're not sure unit wrappers are even helpful because we're using the dictionaries. If we get rid of the unit wrapper, we would have to .to(unit) the user inputs instead. Otherwise, future work should include .to(unit) for every value that the dictionary is passing in to ensure that if the dictionary is updated with the wrong units, we convert it properly. This will likely be messy looking.

Basically we think both options are not ideal, but we have to choose one.

fletchapin commented 6 years ago

To follow up with what @zoemaisel said, this is an example of a case where I think that the unit wrappers provide no benefit on the dictionary input and actually require us to use an extra .magnitude command at the end.

screen shot 2018-03-21 at 4 08 48 pm
eak24 commented 6 years ago

@fletchapin with the method I just posted in #43, you should be able to have only one single file per unit process. If this is the case, then why do we need a wrapper? They would only be useful if we're doing funky stuff with the dict directly within that 'designer' function. However, we'll be breaking the dict down and sending it to the already defined smaller functions, so it is fine (and IMO preferable) to send those along with units. How does that sound?

eak24 commented 6 years ago

@AguaClara/aide_design can you guys post here the most complex units you can think of? I want some examples so I can confirm my regex for the YAML units parser works correctly. Please try to think of how to break it! ie: u.m**3.5/ u.pound_force_per_square_inch*u.kg

fletchapin commented 6 years ago

Tested the python functions in LFOM.py and they're working properly. We've begun to update the other unit processes with dictionaries