search

simphony / simphony-common

The native implementation of the Simphony cuds objects
BSD 2-Clause "Simplified" License
7 stars 5 forks source link

Problem: MATERIAL_ID is removed but simphony depends on it #331

Closed mehdisadeghi closed 7 years ago

mehdisadeghi commented 7 years ago

Solution: remove MATERIAL_ID and the code that is using that

@stefanoborini @tuopuu @roigcarlo @ahashibon please review this.

Related issues: #330 #329

close #303

stefanoborini commented 7 years ago

What do you mean with "simphony depends on it?"

There's a problem with appveyor, and I don't have the powers to restart it, I'll push a neutral commit to restart it.

codecov-io commented 7 years ago

Current coverage is 85.90% (diff: 100%)

Merging #331 into master will increase coverage by 0.08%

@@             master       #331   diff @@
==========================================
  Files           115        115          
  Lines          7231       7259    +28   
  Methods           0          0          
  Messages          0          0          
  Branches        857        867    +10   
==========================================
+ Hits           6206       6236    +30   
+ Misses          789        788     -1   
+ Partials        236        235     -1

Powered by Codecov. Last update f2028b7...39562a5

mehdisadeghi commented 7 years ago

What do you mean with "simphony depends on it?"

I mean it is used in some modules and the tests fail if we remove it.

stefanoborini commented 7 years ago

Do you remember which ones specifically?

mehdisadeghi commented 7 years ago

Do you remember which ones specifically?

simphony/io/tests/test_data_container_table.py, simphony/io/tests/test_h5_cuds.py, simphony/io/tests/test_h5_lattice.py, simphony/io/tests/test_indexed_data_container_table.py

mehdisadeghi commented 7 years ago

@stefanoborini have you reviewed this?

stefanoborini commented 7 years ago

@mehdisadeghi JYULB uses it. What are we supposed to replace it with? Material has no storage for this quantity.

ahashibon commented 7 years ago

Material Id should be moved to wrapper, no need for it anymore.

kemattil commented 7 years ago

@ahashibon is correct: in the new design the "Material ID" is used only internally by the JYULB wrappers.

stefanoborini commented 7 years ago

@ahashibon @kemattil I can't move it to wrapper. The DataContainer accepts only CUBA keys, and apparently the code needs to extract a LatticeNode which contains information about the material_id. LatticeNode only accepts DataContainer "restricted" data, so I have no way of returning this information unless I remove the restriction on datacontainer altogether, or rework LatticeNode to accept an extra_data parameter, with all the potential consequences of an extended API.

ahashibon commented 7 years ago

the information on the node should contain the MATERIAL object it self (or some kind of ref to it). I am afraid we need a bit more time to finalize the specs (will send a review this week with Mehdi). There should be a change in the wrapper side at the same time we change common before the release is possible.

stefanoborini commented 7 years ago

@ahashibon Ok, I leave jyuld for later and move on to something else.