Remove unnecessary indepencence checking

[mjwen]

Remove an unnecessary check on deformations. This will not affect any results, but just doing it in a different way.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -3.22 :warning:

Comparison is base (91ee6a4) 91.22% compared to head (b4091d8) 88.00%.

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     

Impacted Files	Coverage Δ
emmet-core/emmet/core/elasticity.py	85.40% <100.00%> (-0.98%)	:arrow_down:

... and 59 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[tsmathis]

@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:

  # sym op generates a non-independent deform
  if not d_strain.get_deformation_matrix().is_independent(tol=tol):
      continue

leaving that code, but removing the set tol value of 0.002 also causes the same failure

Any clues there? Should those docs be failing now?

[mjwen]

Hi @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 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.

[tsmathis]

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 https://github.com/materialsproject/emmet/pull/767#issuecomment-2364727015, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIPH7AEBVUMQTQKYTXAMS7DZXSRPDAVCNFSM6AAAAABOOOQA6SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRUG4ZDOMBRGU . You are receiving this because you were mentioned.Message ID: @.***>

[mjwen]

Sorry, looking at this closer, so the commit Remove unnecessary indepencence checking was just merged into the main yesterday. Then, the current database was generated by the old code -- then we need to do some comparison for sure.

[tschaume]

@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.

[mjwen]

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.