Change unit_system to code in answer tests
Hi folks, In going through the yt4-merge pull request, Kacper and I tracked down some issues related to the changeover in unyt from using reference values in CGS to using reference values in MKS. Rather than attempt to reconcile these two things (ok actually we *tried* but it turns out it's super duper hard, thanks a LOT floating point math) it seems that the simpler thing to do is to make most of the values compared in the base unit system of "code" rather than CGS. I have issued this pull request: https://github.com/yt-project/yt/pull/2568 to change over the default for the answer value tests to use unit_system="code" if the ds is supplied as a string (rather than an object). Now, if we are concerned that we do not have adequate test coverage for verifying that the units are in CGS, or if we have concerns about this glossing over some CGS conversion factor issues, we should perhaps supplement these tests. This PR will be ready once all of the test storage IDs have been updated, and once I've migrated most/all of the datasets to using unit_system="code", but I wanted to open this up for possible discussion here. It's entirely possible this is not an acceptable solution, and I am hoping that folks may have some feedback. What am I missing? :) Thanks, Matt PS I genuinely think that this might un-clog a whole bunch of issues in the yt4-merge, and we can focus on just verifying the results for the SPH datasets. Hooray!
Hi Matt, I support this change. I think it'll be nicer not having to compare values that are ~1e24 or ~1e-24, etc. The only thing that comes to mind is that testing in typically CGS invokes the comoving to proper cosmology reference frame transform, so we should make sure that doesn't get lost in the shuffle. Britton On Wed, Apr 29, 2020 at 3:52 AM Matthew Turk <matthewturk@gmail.com> wrote:
Hi folks,
In going through the yt4-merge pull request, Kacper and I tracked down some issues related to the changeover in unyt from using reference values in CGS to using reference values in MKS. Rather than attempt to reconcile these two things (ok actually we *tried* but it turns out it's super duper hard, thanks a LOT floating point math) it seems that the simpler thing to do is to make most of the values compared in the base unit system of "code" rather than CGS.
I have issued this pull request:
https://github.com/yt-project/yt/pull/2568
to change over the default for the answer value tests to use unit_system="code" if the ds is supplied as a string (rather than an object).
Now, if we are concerned that we do not have adequate test coverage for verifying that the units are in CGS, or if we have concerns about this glossing over some CGS conversion factor issues, we should perhaps supplement these tests.
This PR will be ready once all of the test storage IDs have been updated, and once I've migrated most/all of the datasets to using unit_system="code", but I wanted to open this up for possible discussion here. It's entirely possible this is not an acceptable solution, and I am hoping that folks may have some feedback. What am I missing? :)
Thanks,
Matt
PS I genuinely think that this might un-clog a whole bunch of issues in the yt4-merge, and we can focus on just verifying the results for the SPH datasets. Hooray! _______________________________________________ yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org https://mail.python.org/mailman3/lists/yt-dev.python.org/
Ah, good point. I'll add a couple tests for these that compare (with reasonably strict, but still present, fuzziness) the unit-converted values for the datasets as well. On Wed, Apr 29, 2020 at 3:46 AM Britton Smith <brittonsmith@gmail.com> wrote:
Hi Matt,
I support this change. I think it'll be nicer not having to compare values that are ~1e24 or ~1e-24, etc. The only thing that comes to mind is that testing in typically CGS invokes the comoving to proper cosmology reference frame transform, so we should make sure that doesn't get lost in the shuffle.
Britton
On Wed, Apr 29, 2020 at 3:52 AM Matthew Turk <matthewturk@gmail.com> wrote:
Hi folks,
In going through the yt4-merge pull request, Kacper and I tracked down some issues related to the changeover in unyt from using reference values in CGS to using reference values in MKS. Rather than attempt to reconcile these two things (ok actually we *tried* but it turns out it's super duper hard, thanks a LOT floating point math) it seems that the simpler thing to do is to make most of the values compared in the base unit system of "code" rather than CGS.
I have issued this pull request:
https://github.com/yt-project/yt/pull/2568
to change over the default for the answer value tests to use unit_system="code" if the ds is supplied as a string (rather than an object).
Now, if we are concerned that we do not have adequate test coverage for verifying that the units are in CGS, or if we have concerns about this glossing over some CGS conversion factor issues, we should perhaps supplement these tests.
This PR will be ready once all of the test storage IDs have been updated, and once I've migrated most/all of the datasets to using unit_system="code", but I wanted to open this up for possible discussion here. It's entirely possible this is not an acceptable solution, and I am hoping that folks may have some feedback. What am I missing? :)
Thanks,
Matt
PS I genuinely think that this might un-clog a whole bunch of issues in the yt4-merge, and we can focus on just verifying the results for the SPH datasets. Hooray! _______________________________________________ yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org https://mail.python.org/mailman3/lists/yt-dev.python.org/
_______________________________________________ yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org https://mail.python.org/mailman3/lists/yt-dev.python.org/
Hi Britton, I spent some time looking at it, and I am now of the opinion that we do have the fuzzy checks for unit-converted fields, as FieldValuesTest gets emitted by most everything, and it only uses the internal yt fields, which have default unit scaling of cgs. It defaults to 10 decimals of precision. I've updated my PR to have all the calls to data_dir_load by default specify unit system = code, which is perhaps a bigger change than I originally intended to make, but I will iterate on it and then modify the tests individually. The plan, if this works, is to then merge this change into the yt4-merge PR, and then we should be able to reduce the number of answer tests that need to be updated. On Wed, Apr 29, 2020 at 8:35 AM Matthew Turk <matthewturk@gmail.com> wrote:
Ah, good point. I'll add a couple tests for these that compare (with reasonably strict, but still present, fuzziness) the unit-converted values for the datasets as well.
On Wed, Apr 29, 2020 at 3:46 AM Britton Smith <brittonsmith@gmail.com> wrote:
Hi Matt,
I support this change. I think it'll be nicer not having to compare values that are ~1e24 or ~1e-24, etc. The only thing that comes to mind is that testing in typically CGS invokes the comoving to proper cosmology reference frame transform, so we should make sure that doesn't get lost in the shuffle.
Britton
On Wed, Apr 29, 2020 at 3:52 AM Matthew Turk <matthewturk@gmail.com> wrote:
Hi folks,
In going through the yt4-merge pull request, Kacper and I tracked down some issues related to the changeover in unyt from using reference values in CGS to using reference values in MKS. Rather than attempt to reconcile these two things (ok actually we *tried* but it turns out it's super duper hard, thanks a LOT floating point math) it seems that the simpler thing to do is to make most of the values compared in the base unit system of "code" rather than CGS.
I have issued this pull request:
https://github.com/yt-project/yt/pull/2568
to change over the default for the answer value tests to use unit_system="code" if the ds is supplied as a string (rather than an object).
Now, if we are concerned that we do not have adequate test coverage for verifying that the units are in CGS, or if we have concerns about this glossing over some CGS conversion factor issues, we should perhaps supplement these tests.
This PR will be ready once all of the test storage IDs have been updated, and once I've migrated most/all of the datasets to using unit_system="code", but I wanted to open this up for possible discussion here. It's entirely possible this is not an acceptable solution, and I am hoping that folks may have some feedback. What am I missing? :)
Thanks,
Matt
PS I genuinely think that this might un-clog a whole bunch of issues in the yt4-merge, and we can focus on just verifying the results for the SPH datasets. Hooray! _______________________________________________ yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org https://mail.python.org/mailman3/lists/yt-dev.python.org/
_______________________________________________ yt-dev mailing list -- yt-dev@python.org To unsubscribe send an email to yt-dev-leave@python.org https://mail.python.org/mailman3/lists/yt-dev.python.org/
participants (2)
-
Britton Smith -
Matthew Turk