search

HEXRD / hexrd

A cross-platform, open-source library for the analysis of X-ray diffraction data.
Other
56 stars 25 forks source link

Single grain fitting #682

Closed kevindlewis23 closed 3 months ago

kevindlewis23 commented 3 months ago

Confirm this correctly understands what grain_table is. This still passes the testcases, and if test_fitgrains is changed to remove the ndmin=2 (and making other necessary changes), it also passes, implying that this change does what it's supposed to do.

closes #677

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 30.65%. Comparing base (b6ae69f) to head (d867986).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #682 +/- ## ======================================= Coverage 30.65% 30.65% ======================================= Files 130 130 Lines 21905 21906 +1 ======================================= + Hits 6714 6715 +1 Misses 15191 15191 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gustafsonsven commented 3 months ago

Was this unit tested against the multi-ruby datasets? I don't see that specific test. Just want to make sure this does not break a multi-grain grains table but changing the shape incorrectly.

kevindlewis23 commented 3 months ago

Was this unit tested against the multi-ruby datasets? I don't see that specific test. Just want to make sure this does not break a multi-grain grains table but changing the shape incorrectly.

I don't think there are any testcases for the multi-ruby sets. The compare_grain_fits method in tests/fit_grains_check.py even calls np.atleast_2d on the inputs anyway. With the multi-ruby datasets, this will already be 2d and this statement won't have any effect, but I understand not merging until we have those testcases.

We probably want to have unit tests for something other than the single ge detector anyway.

gustafsonsven commented 3 months ago

Ah I see. Since it will not have any effect, and it allows a single grain to be fit correctly, then I don't see why we cannot push this through. We should create an artificial mruby test case with a single grain just be eliminating the other two grains in the grains.out estimate.

psavery commented 3 months ago

The only fit grains test we have is for the single ruby GE example, which actually only has one grain. However, it may be called in a different way such that a 2D grains_table is passed and avoids the error you were seeing @gustafsonsven. This change could potentially fix your issue while not affecting other cases. I'll approve and merge it.