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

Change timestepping structure in run_part #176

Merged
merged 14 commits into from
Aug 6, 2023
Merged

Conversation

jcurtis2
Copy link
Collaborator

@jcurtis2 jcurtis2 commented Sep 4, 2022

run_part both controls the timestepping and calls all the functions individually to progress an entire simulation (from time $t=0$ to $t=t_{\rm max}$. Instead it would be good to be able to run chunks of time, implemented with run_part_timeblock which will simulate from time $t$ to $t + t_{\rm progress}$ (or other specified interval), and a single timestep, implemented with run_part_timestep which runs from time $t$ to $t+\Delta t$.

Copy link
Contributor

@mwest1066 mwest1066 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 to me! I left suggestions about separating out the CAMP arguments on a fine-grained level. This isn't required but I think it makes the code a bit clearer because it highlights what is actually different with CAMP. It might also make it simpler with future modifications because we only need to change one place rather than two.

src/run_part.F90 Outdated
Comment on lines 480 to 492
#ifdef PMC_USE_CAMP
subroutine run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, camp_core, photolysis, i_time, &
t_start, last_output_time, last_progress_time, i_output, &
progress_n_samp, progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
#else
subroutine run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, i_time, t_start, last_output_time, &
last_progress_time, i_output, progress_n_samp, progress_n_coag, &
progress_n_emit, progress_n_dil_in, progress_n_dil_out, &
progress_n_nuc)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner (and safer for future modifications) to separate out the CAMP code on a more fine-grained level? This would match what we do below in the argument variable definitions. For example:

Suggested change
#ifdef PMC_USE_CAMP
subroutine run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, camp_core, photolysis, i_time, &
t_start, last_output_time, last_progress_time, i_output, &
progress_n_samp, progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
#else
subroutine run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, i_time, t_start, last_output_time, &
last_progress_time, i_output, progress_n_samp, progress_n_coag, &
progress_n_emit, progress_n_dil_in, progress_n_dil_out, &
progress_n_nuc)
#endif
subroutine run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, &
#ifdef PMC_USE_CAMP
camp_core, photolysis, &
#endif
i_time, &
t_start, last_output_time, last_progress_time, i_output, &
progress_n_samp, progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)

src/run_part.F90 Outdated
Comment on lines 705 to 717
#ifdef PMC_USE_CAMP
subroutine run_part_timeblock(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, camp_core, photolysis, i_cur, &
i_next, t_start, last_output_time, last_progress_time, i_output, &
progress_n_samp, progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
#else
subroutine run_part_timeblock(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, i_cur, i_next, t_start, &
last_output_time, last_progress_time, i_output, progress_n_samp, &
progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do a similar separation of just the CAMP arguments here.

src/run_part.F90 Outdated
Comment on lines 766 to 778
#ifdef PMC_USE_CAMP
call run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, camp_core, photolysis, i_time, &
t_start, last_output_time, last_progress_time, i_output, &
progress_n_samp, progress_n_coag, progress_n_emit, &
progress_n_dil_in, progress_n_dil_out, progress_n_nuc)
#else
call run_part_timestep(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, i_time, t_start, &
last_output_time, last_progress_time, i_output, progress_n_samp, &
progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here we could separate out just the CAMP arguments.

src/run_part.F90 Outdated
Comment on lines 232 to 242
call run_part_timeblock(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, camp_core, photolysis, &
i_cur, i_next, t_start, last_output_time, last_progress_time, &
i_output, progress_n_samp, progress_n_coag, progress_n_emit, &
progress_n_dil_in, progress_n_dil_out, progress_n_nuc)
#else
call run_part_timeblock(scenario, env_state, aero_data, aero_state, &
gas_data, gas_state, run_part_opt, i_cur, i_next, t_start, &
last_output_time, last_progress_time, i_output, progress_n_samp, &
progress_n_coag, progress_n_emit, progress_n_dil_in, &
progress_n_dil_out, progress_n_nuc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also separate out the CAMP arguments here.

@jcurtis2 jcurtis2 merged commit e73f653 into master Aug 6, 2023
@jcurtis2 jcurtis2 deleted the change_timestepping branch August 6, 2023 19:40
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.

2 participants