Closed mabruzzo closed 3 months ago
@brittonsmith, when you have a chance, I think this PR probably merits some consideration, given the conclusions reached in #216, (that the comoving unit-system is entirely parameterized by the code_units struct used at initialization and the current scale_factor).
(But definitely don’t feel rushed)
There's a minor issue with the test that checks for consistency between the values accessible from the dynamic API and the chemistry_data
type. This test parses the chemistry_data
type from grackle_chemistry_data.h
.
grackle_chemistry_data.h
didn't have any include-directives. (There were no problems)code_units
inside of chemistry_data_storage
. Since chemistry_data_storage
is defined inside chemistry_data.h
and code_units
is defined inside of grackle_types.h
, I needed to introduce the line #include "grackle_types.h"
into the grackle_chemistry_data.h
file. (This worked at the time).grackle_float.h
file from the include-directory where the other public headers are found into a newly generated directory called src/clib/autogen/
. This causes problems because grackle_types.h
must include the grackle_float.h
directory and the parser can't find it.We could adopt a simple fix: tell the parser of grackle_chemistry_data.h
that it can find headers inside of src/clib/autogen
.
But, this isn't a particularly robust solution since:
src/clib/autogen/grackle_chemistry_data.h
can be deleted while maintaining a valid pygrackle installation (which would break the tests) While there are some alternatives that avoid these particular issues[^1], I think this is a symptom of a deeper problem beyond the scope of this PR. Similar issues arise related to the test_code_examples
test (if we delete build-artifacts those tests will start failing; we also need to introduce a special "hack" to get those tests to run with the cmake-build system introduced in #182). The issue is that these tests fundamentally assume that they are being run without any changes being made to the underlying build-tree. I had some ideas for some additional tests I want to introduce, but they will be subject to similar problems. This hasn't been an issue until now, but it's probably something we need to consider now that we are thinking about shipping Pygrackle
.
I'm tempted to adopt the "simple-fix" and plan to skip this test in the cmake-build system right now (I think a more-thorough solution requires a little more thought) - but I only want to do that if we are okay merging #182 without this test.
[^1]: We could introduce some ifdef statements. Or we could move code_units
from grackle_types.h
into grackle_chemistry_data.h
or its own header. (By doing that, we would eliminate the presence of #include "grackle_types.h"
from grackle_chemistry_data.h
)
So this should all be fixed. Since you all looked at this, I essentially made 3 changes:
gr_required_units
-> gr_query_units
(as @gregbryan suggested)GR_SPECIFY_INITIAL_A_VALUE
constant to be used to tell the gr_query_units
function that we want to query the initial a_value.
GR_SPECIFY_INITIAL_A_VALUE
as an argument rather than manually passing -1
as that function argument. This is done in case we want to switch to using a value other than -1
for this value in the future.GR_SPECIFY_INITIAL_A_VALUE
as a macro. But, we can always change that later without breaking the APItest_chemistry_struct_synched.py
test. It turns out there was a really simple solution staring me right in the face.
Background
Managing the
code_units
data structure is a little tricky! Issue #198 proposes that we provide functionality for grackle to automatically manage the data associated with thecode_units
behind the scenes. While I still think that is a fantastic idea, there are still some implementation questions that must be addressed before we can proceed with that.[^1]This PR starts to introduce some of the associated machinery and a utility function that can probably help out now.
Description
This PR does 2 things.
First, it stores a copy of the
code_units
information within thechemistry_data_storage
data structure during initialization. This is primarily done to help out with the next part, but it may help with other unrelated things.[^2]Second, it introduces the following function to the public API:
This function will return the expected units quantity given an initialized
chemistry_data_storage
instance for an arbitrarya_value
."a_value"
,"a_units"
,"density_units"
,"length_units"
,"temperature_units"
,"time_units"
,"velocity_units"
.current_a_value
is-1
, then grackle assumes that the user wants to use the initial"a_value"
as their choice.-1
is returned."a_value"
to be a valid argument. While it would be useful to query the initial choice of a_value, maybe we should only do thatcurrent_a_value
andunits_name == NULL
. Feedback on this particular point would be appreciatedThe main selling point is that downstream developers can query what grackle expects the units to be (mostly as a sanity check!)
[^1]: For example, do we track the current
a_value
as a member of thegrackle_field_data
struct? There are other options too. Alternatively, we could provide a functionupdate_current_a_value(chemistry_data_storage* my_rates, double current_a_value)
that tracks this information internally. (I strongly prefer the former for stylistic reasons, but the latter could potentially facilitate better performance). In any case, this is a discussion to be had in #198.[^2]: This change would also let us drastically simplify the usage of the debugging function introduced in #197. At the moment, that function requires the user to separately track the initial choice of the
code_units
struct. Now, that could be tracked automatically. With that said, we would need to modify the function interface (but that's perfectly ok)