Skip to content
This repository was archived by the owner on Apr 23, 2021. It is now read-only.

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

Merged
stefanoborini merged 3 commits into
masterfrom
remove-material-id
Oct 26, 2016
Merged

Problem: MATERIAL_ID is removed but simphony depends on it#331
stefanoborini merged 3 commits into
masterfrom
remove-material-id

Conversation

@mehdisadeghi

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

codecov-io commented Oct 25, 2016

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

Do you remember which ones specifically?

@mehdisadeghi

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

@stefanoborini have you reviewed this?

@stefanoborini

Copy link
Copy Markdown
Contributor

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

@ahashibon

ahashibon commented Jan 12, 2017 via email

Copy link
Copy Markdown
Contributor

@kemattil

Copy link
Copy Markdown
Contributor

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

@stefanoborini

Copy link
Copy Markdown
Contributor

@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

ahashibon commented Jan 12, 2017 via email

Copy link
Copy Markdown
Contributor

@stefanoborini

Copy link
Copy Markdown
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clean up material_id from simphony-common

5 participants