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

Simplify heating source usage. #21

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

aprilnovak
Copy link
Contributor

Closes #17

@helen-brooks
Copy link
Contributor

Thanks April. Before we can merge this:

  • I need to first get in the check that examples run in CI.
  • I wonder if we also should add a "gold" results file for the examples. I'm hesitant to do this, since the examples ares not based on any physics, but I would like to know if there is a change from doing it this way.
  • I need to decide if there is an argument to keep the legacy way of doing things. If there is not, we should also remove the dead code, namely FunctionUserObject and VariableFunction, and their unit tests. Have you checked this method for a few different cases? E.g. second order, refined, displaced? There's a whole host of unit tests in aurora/unit that check all theses cases. I wonder if we should update or duplicate these.

@aprilnovak
Copy link
Contributor Author

  • Personally, I think it would be a good idea to have the gold files. I don't interpret gold files as verification/validation cases, just as ways to ensure that code results are reproducible (even if the physics themselves isn't particularly meaningful).
  • I think the legacy approach should be removed - the other approach is less input file syntax, and should be clearer how to control when things get executed (instead of having object A depend on object B depend on object C depend on object D). Plus there should be less overall tests you need to retain. (all this is with the caveat that the approaches are exactly identical, that is)

@helen-brooks
Copy link
Contributor

  • I need to first get in the check that examples run in CI.
  • I wonder if we also should add a "gold" results file for the examples. I'm hesitant to do this, since the examples ares not based on any physics, but I would like to know if there is a change from doing it this way.
  • I need to decide if there is an argument to keep the legacy way of doing things. If there is not, we should also remove the dead code, namely FunctionUserObject and VariableFunction, and their unit tests. Have you checked this method for a few different cases? E.g. second order, refined, displaced? There's a whole host of unit tests in aurora/unit that check all theses cases. I wonder if we should update or duplicate these.

I'll think I'm now ready to come back to this PR. The main prerequisites are in place.
#24 Brings the examples into CI
#26 Moves the examples into tests and adds gold files

I think I now know how to handle this PR. I think we retain the "legacy" way of doing things, but label it as such. In one direction at least there is value to with the convoluted route as I think you could nominally tally on a different mesh than the one you store the values on, because under the hood I am using a the mesh function which uses a point locator. Combined with DAGMC universes this might be useful in future if we ever just want the heating field on a single component. Essentially I don't want to deprecate this code yet, as it may have value yet. And therefore, it still needs to be tested.

However, I totally agree that for basic usage, your way is much cleaner. So, I propose the following.

  • Keep one example using the legacy route. E.g. copy tests/thermal --> tests/thermal-legacy
  • You update tests/thermal/main.i and tests/thermal-mech/main_temp_mech.i (i.e. just move your files into the test directory)
    This will automatically check the output against the gold files which was generated with the legacy route. If the outputs are different, we'll have to understand why.

Does that sound OK?

@aprilnovak
Copy link
Contributor Author

Sorry for the delay, I was out on vacation.

I think this plan sounds great! It will be nice to have some CI covering the input file syntax. I'll make the requested changes in this PR.

@aprilnovak
Copy link
Contributor Author

@helen-brooks this is ready for review. I created two new test directories with the "simplified" input file syntax. For everything except the input files, I created sym links to the "legacy" versions, and then added just an exodiff.cmp file that ignores the volumetric_heat variable (which is no longer created with the "simplified" syntax). This means that these tests are exactly reproducing the "legacy" approach with identical gold files.

Copy link
Contributor

@helen-brooks helen-brooks left a comment

Choose a reason for hiding this comment

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

Thanks April. looks good. The only issue is I don't think the exodiff files generated by the tests should be included. Delete those and we should be good to go.

@aprilnovak
Copy link
Contributor Author

Not sure I follow - the only gold files added were the renamed ones (moving from thermal/gold to thermal-legacy/gold, for instance). The other two are just sym links to those files.

@helen-brooks
Copy link
Contributor

Not sure I follow - the only gold files added were the renamed ones (moving from thermal/gold to thermal-legacy/gold, for instance). The other two are just sym links to those files.

Two files named exodiff.cmp which are generated by the tests were erroneously commited:

https://github.com/aurora-multiphysics/aurora/blob/b4e82c082135f9449efe3b6bb0ff4ffc7819d527/test/tests/thermal-mech/exodiff.cmp

and

https://github.com/aurora-multiphysics/aurora/blob/b4e82c082135f9449efe3b6bb0ff4ffc7819d527/test/tests/thermal/exodiff.cmp

Please could you delete them?

@aprilnovak
Copy link
Contributor Author

aprilnovak commented Oct 18, 2022

No, the exodiff.cmp are not created by the tests. Those are extra configuration files for the ExoDiff utility. Those are needed in order for the "modern" tests to properly ignore a variable created to hold a "middle-man" Material that is created when using the legacy approach.

Otherwise, the "modern" tests will fail with a message like "Could not find variable local-heating in file 2, but it is present in file 1"

@helen-brooks
Copy link
Contributor

No, the exodiff.cmp are not created by the tests. Those are extra configuration files for the ExoDiff utility. Those are needed in order for the "modern" tests to properly ignore a variable created to hold a "middle-man" Material that is created when using the legacy approach.

Otherwise, the "modern" tests will fail with a message like "Could not find variable local-heating in file 2, but it is present in file 1"

Ah, my bad! In that case all looks good!

@helen-brooks helen-brooks merged commit 3772188 into aurora-multiphysics:main Oct 18, 2022
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.

Simplify usage of heating-local variable
2 participants