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

220 config file csv - initial chem species are moved to CSV, and also specified with JSON data #299

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

carl-drews
Copy link
Collaborator

my_config.json will now initialize chemical species like this JSON specifying one or more CSV files:

  "initial conditions": {
    "filepaths": ["initial_conditions.csv"],
    "data": [
      ["ENV.temperature [K]", "ENV.pressure [Pa]", "CONC.A [mol m-3]", "CONC.B [mol m-3]"],
      [200, 70000, 0.8, 0.2]
    ]
  },

Evolving conditions are also specified in the same manner:

  "evolving conditions": {
    "filepaths": [
      "conditions_Boulder.csv"
    ]
  },

The implementation provides warnings when a later condition overrides an earlier one.

carl-drews added 30 commits December 9, 2024 16:56
Comment on lines +109 to +114
@classmethod
def retrieve_initial_conditions_from_JSON(
self,
path_to_json,
json_object,
reaction_types):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@classmethod
def retrieve_initial_conditions_from_JSON(
self,
path_to_json,
json_object,
reaction_types):
@classmethod
def retrieve_initial_conditions_from_JSON(
cls,
path_to_json,
json_object,
reaction_types):

I only recently learned about classmethod and staticmethod. But, apparently it's common in python to use cls instead of self for classmethods.

Comment on lines +321 to +328
for header, value in zip(header_row, value_row):
# are we looking for this type?
if (react_types):
header_type = header.split('.')[0]
if (header_type not in react_types):
continue

data_values[header] = float(value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for header, value in zip(header_row, value_row):
# are we looking for this type?
if (react_types):
header_type = header.split('.')[0]
if (header_type not in react_types):
continue
data_values[header] = float(value)
df = pd.DataFrame(value_row, columns=header_row)

I think pandas may be able to handle this. In general I think we should prefer to push parsing into pandas as much as possible

Comment on lines +18 to +21
"filepaths": [
"initial_reaction_rates.csv",
"initial_concentrations.csv"
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love that we can do this now!

@@ -6,7 +6,7 @@
"output time step [sec]": 60.0,
"simulation length [hour]": 1.0
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"chemical species deprecated": {

If we are doing to deprecate the thing, I think we should remove it entirely and we will update music box interactive accordingly to work with this new configuration format. We only have one active user whose configurations we will need to update.

@@ -6,7 +6,7 @@
"output time step [sec]": 1,
"simulation length [sec]": 100
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -6,7 +6,7 @@
"output time step [sec]": 1,
"simulation length [sec]": 100
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -6,7 +6,7 @@
"output time step [sec]": 1,
"simulation length [sec]": 100
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -6,7 +6,7 @@
"output time step [sec]": 1,
"simulation length [sec]": 100
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

@@ -6,7 +6,7 @@
"output time step [sec]": 1,
"simulation length [sec]": 100
},
"chemical species": {
"chemical species deprecated": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Comment on lines +79 to +89
C_conc += initial_A * (
1.0
+ (k1 * math.exp(-k2 * curr_time) - k2 * math.exp(-k1 * curr_time))
/ (k2 - k1)
)
) + initial_B

if (curr_time >= last_out_time + out_time_step):
analytical_concentrations.append([A_conc, B_conc, C_conc])
last_out_time += out_time_step

analytical_concentrations.append([A_conc, B_conc, C_conc])
curr_time += time_step
curr_time += chem_time_step
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be accumulating rather than transforming the mass. I don't think that is correct, and I don't understand the new time stuff. Can you explain that a bit more?

Copy link
Collaborator

@mattldawson mattldawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good! just a few questions and suggestions

Comment on lines +218 to +221
species_concentrations = Conditions.retrieve_initial_conditions_from_JSON(
path_to_json, json_object, {"ENV", "CONC"})
reaction_rates = Conditions.retrieve_initial_conditions_from_JSON(
path_to_json, json_object, {"EMIS", "PHOTO", "LOSS"})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would separate environmental conditions from species concentrations here, just for clarity

Comment on lines +6 to +7
"simulation length [sec]": 100.0,
"real simulation length [hr]": 1.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between simulation length and real simulation length?

A_conc = initial_A * math.exp(-(k1) * curr_time)
B_conc = (
initial_A
* (k1 / (k2 - k1))
* (math.exp(-k1 * curr_time) - math.exp(-k2 * curr_time))
)
C_conc = initial_A * (
C_conc += initial_A * (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
C_conc += initial_A * (
C_conc = initial_A * (

The analytical examples can be calculated from the initial concentrations and the current time, so no need to accumulate the values.

1.0
+ (k1 * math.exp(-k2 * curr_time) - k2 * math.exp(-k1 * curr_time))
/ (k2 - k1)
)
) + initial_B
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this added?

A_conc = initial_A * math.exp(-(k1) * curr_time)
B_conc = (
initial_A
* (k1 / (k2 - k1))
* (math.exp(-k1 * curr_time) - math.exp(-k2 * curr_time))
)
C_conc = initial_A * (
C_conc += initial_A * (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
C_conc += initial_A * (
C_conc = initial_A * (

1.0
+ (k1 * math.exp(-k2 * curr_time) - k2 * math.exp(-k1 * curr_time))
/ (k2 - k1)
)
) + initial_B
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants