-
Notifications
You must be signed in to change notification settings - Fork 66
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
Remove unnecessary indepencence checking #767
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #767 +/- ##
==========================================
- Coverage 91.22% 88.00% -3.22%
==========================================
Files 151 108 -43
Lines 11948 7221 -4727
==========================================
- Hits 10899 6355 -4544
+ Misses 1049 866 -183
☔ View full report in Codecov by Sentry. |
@mjwen, checking the output from the builder tests now shows that the elasticity builder now fails (deprecates) 4 out the expected 6 elasticity docs traced it back to this chunk of code that was removed:
leaving that code, but removing the set Any clues there? Should those docs be failing now? |
# sym op generates a non-independent deform | ||
if not d_strain.get_deformation_matrix().is_independent(tol=tol): | ||
continue | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code mentioned in the main conversation
Hi @tsmathis:
I've taken a look into this and tried to recall what happened. The new code should be good, and I believe I forgot to update the tests. So, these docs should fail. I cannot remember whether the old or the new code was used to generate the current elastic database. @tschaume may have an idea. But to ensure everything is alright, maybe we can build for a subset of materials and compare the values with the current database? I guess we don't want to put out bad data. |
Sounds good, we can update the tests.
…On Fri, Sep 20, 2024 at 3:53 PM Mingjian Wen ***@***.***> wrote:
Hi @tsmathis <https://github.com/tsmathis>:
Any clues there? Should those docs be failing now?
I've taken a look into this and tried to recall what happened. The new
code should be good, and I believe I forgot to update the tests. So, these
docs should fail.
I cannot remember whether the old or the new code was used to generate the
current elastic database. @tschaume <https://github.com/tschaume> may
have an idea. So, to ensure everything is alright, maybe we can build for a
subset of materials and compare it with the current database? I guess we
don't want to put out bad data.
—
Reply to this email directly, view it on GitHub
<#767 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIPH7AEBVUMQTQKYTXAMS7DZXSRPDAVCNFSM6AAAAABOOOQA6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRUG4ZDOMBRGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry, looking at this closer, so the commit |
@mjwen yes, I decided to merge this yesterday. It sounded from the PR as if it shouldn't make a difference for the data. The build validation with our next release should hopefully reveal any potential differences. If we can't explain them, I'd be happy to roll it back and start a new PR for you to work on. |
Sounds good! @tschaume Yes, it should not make any difference. But it can depend on how we build it, that is, what tasks we supply to the materials builder that the elastic builder depends on. Below I write down the reason to remind myself if it turns out we need a debug. I remember you or Matt told me that we merge all tasks from different calculations (e.g. elastic, thermal, magnetic...) together, and then identify what tasks belong to a material. I might have misunderstood it, but if this is true, there can be an issue that tasks not related to elastic calculations are provided to the elastic builder, which can then result in a failure. The couple of lines I removed tries to avoid such situations, but I don't believe it should belong to the elastic builder. That's why I removed it. |
Remove an unnecessary check on deformations. This will not affect any results, but just doing it in a different way.