Add error-check on grackle_field_data.grid_dx
#190
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `grid_dx` field is rarely needed for grackle operations and it sometimes doesn't make sense for a downstream application to properly set the value (e.g. where cells are not perfect cubes or if the code isn't grid-based). As part of this commit, I actually factored the error-check out into it's own function (as well as another related check) in order to make sure that we apply the same test in both the `calculate_cooling_time` and `solve_chemistry` functions.
mabruzzo
added
enhancement
brittonsmith
approved these changes
Apr 3, 2024
brittonsmith
added a commit
that referenced
this pull request
Apr 8, 2024
Fix minor oversight from #190
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this pull request
Jun 3, 2024
This change was split off from GH PR grackle-project#197. This should be a lot easier to review in isolation (and I think that it is a generally useful change in its own right). Overview -------- The objective of this PR is conceptually very simple. It introduces the following function: ```c++ int gr_initialize_field_data(grackle_field_data *my_fields); ``` The idea is to have users immediately call this function right after they initialize a new `grackle_field_data` instance - When it's called the handful of non data-field members are assigned sensible defaults. It also initializes all data-field members to NULL pointers. - this is analogous to the way that `local_initialize_chemistry_parameters` is used. Motivation ---------- While this function is not strictly necessary, I think it is a useful addition and I think we should tell people to use it (though old code won't break). A large motivation is peace of mind. I have often wished for this sort of thing. I'm always a little concerned when using a new configuration of grackle that I didn't initialize all of the required fields. I would definitely feel a little more comfortable knowing that a field that I forgot about is assigned a ``NULL`` pointer rather than some garbage data that might let the code chug along. It could also facillitate more error checking: - such as checks in the style of the ``grid_dx`` check introduced in PR grackle-project#190 (applying similar checks to `grid_rank`, `grid_dimension`, `grid_start`, and `grid_end` requires that we know their default value). - we could explicitly check that the user specified all required data-fields. We can only do this if we know that unset data-fields have a default value of `NULL`. - **From a user friendliness perspective, I think this check alone is enough to justify the function's existence.** - We could also potentially warn if unnecessary fields were specified - Plus, we could account for the fact that functions like ``calculate_pressure`` require fewer fields that ``solve_chemistry`` - we could also add checks that none of the fields are aliases (an implicit Fortran requirement). This would be much easier to implement if we knew we could just ignore fields that were set to ``NULL`` pointers We would probably need to make these checks opt-in somehow, both because we can't be absolutely certain whether a user actually invoked ``gr_initialize_field_data`` and users might not want the runtime-cost (which could be relatively large for very small grids) Naming ------ I choose to name this function based on my suggestion in grackle-project#189 that we adopt a convention that all new functions in the stable public API share a common prefix (`gr_` or `grackle_`). For the sake of proposing something concrete, I assumed that we choose the `gr_` prefix. (If we decide on a different prefix, this would be easy to change). Alternatively, if we don't want any prefix, we could name it `initialize_grackle_field_data` (I do worry a little that could introduce naming conflicts with existing functions in downstream codes)
mabruzzo
added a commit
to mabruzzo/grackle
that referenced
this pull request
Jun 3, 2024
This change was split off from GH PR grackle-project#197. This should be a lot easier to review in isolation (and I think that it is a generally useful change in its own right). Overview -------- The objective of this PR is conceptually very simple. It introduces the following function: ```c++ int gr_initialize_field_data(grackle_field_data *my_fields); ``` The idea is to have users immediately call this function right after they initialize a new `grackle_field_data` instance - When it's called the handful of non data-field members are assigned sensible defaults. It also initializes all data-field members to NULL pointers. - this is analogous to the way that `local_initialize_chemistry_parameters` is used. Motivation ---------- While this function is not strictly necessary, I think it is a useful addition and I think we should tell people to use it (though old code won't break). A large motivation is peace of mind. I have often wished for this sort of thing. I'm always a little concerned when using a new configuration of grackle that I didn't initialize all of the required fields. I would definitely feel a little more comfortable knowing that a field that I forgot about is assigned a ``NULL`` pointer rather than some garbage data that might let the code chug along. It could also facillitate more error checking: - such as checks in the style of the ``grid_dx`` check introduced in PR grackle-project#190 (applying similar checks to `grid_rank`, `grid_dimension`, `grid_start`, and `grid_end` requires that we know their default value). - we could explicitly check that the user specified all required data-fields. We can only do this if we know that unset data-fields have a default value of `NULL`. - **From a user friendliness perspective, I think this check alone is enough to justify the function's existence.** - We could also potentially warn if unnecessary fields were specified - Plus, we could account for the fact that functions like ``calculate_pressure`` require fewer fields that ``solve_chemistry`` - we could also add checks that none of the fields are aliases (an implicit Fortran requirement). This would be much easier to implement if we knew we could just ignore fields that were set to ``NULL`` pointers We would probably need to make these checks opt-in somehow, both because we can't be absolutely certain whether a user actually invoked ``gr_initialize_field_data`` and users might not want the runtime-cost (which could be relatively large for very small grids) Naming ------ I choose to name this function based on my suggestion in grackle-project#189 that we adopt a convention that all new functions in the stable public API share a common prefix (`gr_` or `grackle_`). For the sake of proposing something concrete, I assumed that we choose the `gr_` prefix. (If we decide on a different prefix, this would be easy to change). Alternatively, if we don't want any prefix, we could name it `initialize_grackle_field_data` (I do worry a little that could introduce naming conflicts with existing functions in downstream codes)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
grid_dxfield is only needed for a subset of grackle operations and it sometimes doesn't make sense for a downstream application to properly set the value (e.g. where cells are not perfect cubes or if the code isn't grid-based).I also tweaked the documentation to recommend that downstream applications set
grackle_field_data.grid_dxto something-1in cases where it's not applicable (if the value is not initialized at all, the new error check might not work...)As part of this commit, I actually factored the error-check out into it's own function (as well as another related check) in order to make sure that we apply the same test in both the
calculate_cooling_timeandsolve_chemistryfunctions.I placed that function inside of a newly created file called
utils.c. I think that could be a good place to place functions that represent duplicated logic present in more than 1 c-routine (e.g. resetting the UVbackground rates, calculating comoving length/density units) and maybe some other basic error checks