Skip to content

cleanup calc_tdust_3d_g#20

Closed
mabruzzo wants to merge 36 commits into
brittonsmith:gen2024from
mabruzzo:gen2024-cleanup_calc_tdust_3d_g
Closed

cleanup calc_tdust_3d_g#20
mabruzzo wants to merge 36 commits into
brittonsmith:gen2024from
mabruzzo:gen2024-cleanup_calc_tdust_3d_g

Conversation

@mabruzzo

@mabruzzo mabruzzo commented Jan 5, 2025

Copy link
Copy Markdown

This needs to be reviewed after #13 (which had resolved the flaking tests in the test-suite)


While working on #19 (not required for this PR), I became aware that there were a bunch of extra unused variables in calc_tdust_3d_g and variables that were defined with type R_PREC when they should be defined as real*8.1 The commits in this PR go through and cleans this all up. I recommend that you review commit-by-commit (this helps a lot)

I have manually confirmed that this PR passes all tests

Footnotes

  1. I became aware of these issues as I was trying to group together local variables into structs and searching through the codebase. At that point I became aware that a bunch of variables were declared, but unused in this file. This also made me aware of the typing mismatch.

this parallel clause is used when converting from comoving to proper
this parallel clause is used when converting from proper to comoving
Removed unused local variables from `calc_tdust_3d_g` that followed the
naming scheme used in solve_rate_cool for tracking the mass density for
the mass densities of various metal species used in dust chemistry.

There was logic to convert between proper and comoving units for each of
these variables, but they were not actually used for anything (which is
a good thing since they were never initialized).

These variable declarations were probably blindly copied from somewhere
else and were never removed.
Removed unused local variables from `calc_tdust_3d_g` that followed the
naming scheme used in solve_rate_cool for tracking the mass density for
tracking the mass densities of various metal species used in metal
chemistry.

There was logic to convert between proper and comoving units for each of
these variables, but they were not actually used for anything (which is
a good thing since they were never initialized).

These variable declarations were probably blindly copied from somewhere
else and were never removed.
@mabruzzo mabruzzo changed the base branch from master to gen2024 January 28, 2025 15:42
This creates problems when we specify the answer-dir as a relative path
because we commonly forget to change the directory back to the starting
one. This issue will become more prominent in the next commit.

- there wasn't any need to change directories in test_initialisation.py
- we could remove all logic relating to explicitly changing directories
  by passing the ``cwd`` kwarg to ``subprocess.run`` in
  test_get_grackle_version.py and test_models.py (while the directory is
  still changed by ``subprocess.run``, they always restore the original
  one afterwards)

More importantly, this is a good practice to have!
Previously, the test_code_examples would eagerly FAIL, if pytest
was invoked to run answer-tests in "compare-mode" and the answer-file
could not be found for the test_code_examples (whether or not we were
planning to skip the test_code_examples).

This commit relocates the FAILURE check until after the logic
determining whether we should skip the test_code_examples.
1. A bug is fixed in the Fortran Bindings
2. All references have been removed about the ``GRACKLE_DATA_DIR`` env
   variable. An upcoming PR (grackle-project#235) is going to overhaul this machinery,
   but it probably won't be merged the 3.4 release
@mabruzzo

Copy link
Copy Markdown
Author

superseded by grackle-project#276

@mabruzzo mabruzzo closed this Mar 20, 2025
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