ABCCheckLattice assumes that e.g. get_node returns empty LatticeNodes

[tuopuu]

ABCheckLattice template assumes that a lattice initiated for testing has no CUBA data stored in it's LatticeNode objects. If the lattice is initiated in the container_factory method with, say, CUBA.MATERIAL_ID defined for all nodes, then CheckLatticeNodeOperations.test_get_node causes an error because it assumes that the returned LatticeNode (node) has an empty DataContainer object as node.data.

JYULatticeProxy lattice has by default certain CUBA keywords defined for the lattice, so using a get_node always returns LatticeNodes that have non-empty DataContainer object accompanied with it. This is currently restricting us of using the test template for JYULatticeProxy.

I propose that ABCheckLattice template is modified so that it does not expect empty nodes returned from get_node, and methods alike. The change can be done by modifying the template, or compare_data_containers and compare_lattice_nodes methods from simphony.testing.utils.py

[nathanfranklin]

I propose that ABCheckLattice template is modified so that it does not expect empty nodes returned from get_node, and methods alike. The change can be done by modifying the template, or compare_data_containers and compare_lattice_nodes methods from simphony.testing.utils.py

Could the fix just be changing the following line https://github.com/simphony/simphony-common/blob/master/simphony/testing/abc_check_lattice.py#L183 from:

node.data = create_data_container()
to
node.data = create_data_container(restrict=self.supported_cuba())

This is currently restricting us of using the test template for JYULatticeProxy.

As a work-around, could you still use CheckLatticeNodesOperations and just override the test_get_node method (like https://github.com/simphony/simphony-lammps-md/blob/14474e5e118a9233d9c4331b445c2ccd50bec6f0/simlammps/tests/test_particles.py#L66_L80) so that you can still use the current version ABCCheckLattice testing template

[tuopuu]

@nathanfranklin, thanks for the ideas!

Could the fix just be changing the following line

Unfortunately this breaks the template for other lattice classes, Lattice and H5Lattice.

I have tried to avoid overriding for now, because I would have to override quite many tests if I went that road. But I'm still testing other ways of solving this. One of the other ways might be to get rid of JYULatticeProxy for good and replace it with a regular Lattice class, that is stored internally in the engine.

One off-topic question: Is it required generally in Simphony, that engine statedata must be able to hold many datasets? Currently JyuLB is only supporting one lattice as its stored dataset. Looking at the engine test template it seems that many datasets should be supported (see e.g. abc_check_engine.iter_datasets test), but I could not find a clear specification from the wiki. I had to override some tests for JyuLB wrappers to be able to use this test template...

[nathanfranklin]

I have tried to avoid overriding for now, because I would have to override quite many tests if I went that road.

Do you mean other tests in ABCheckLattice?

[nathanfranklin]

I propose that ABCheckLattice template is modified so that it does not expect empty nodes returned from get_node, and methods alike. The change can be done by modifying the template, or compare_data_containers and compare_lattice_nodes methods from simphony.testing.utils.py

Could the fix just be changing the following line https://github.com/simphony/simphony-common/blob/master/simphony/testing/abc_check_lattice.py#L183 from:
node.data = create_data_container()
to
node.data = create_data_container(restrict=self.supported_cuba())

Oops. I quickly read the test..and misunderstood what it was testing. The line in question should remain as it is:
node.data = create_data_container()

It is changing the node's data to an empty data container and then making sure that doesn't have an affect in the node stored in the wrapper (i.e. it gets the node again and tests the that changed node doesn't equal the newly acquired node).

[nathanfranklin]

I just added a comment on simphony-jyulb (simphony/simphony-jyulb@a79e935#commitcomment-14072222) that might address the problem encountered with the test_get_node

[nathanfranklin]

One off-topic question: Is it required generally in Simphony, that engine statedata must be able to hold many datasets? Currently JyuLB is only supporting one lattice as its stored dataset. Looking at the engine test template it seems that many datasets should be supported (see e.g. abc_check_engine.iter_datasets test), but I could not find a clear specification from the wiki. I had to override some tests for JyuLB wrappers to be able to use this test template...

I don't know what the general requirement is... currently,we have simphony-jyulb(and I think simphony-openfoam) allowing one to have a single dataset while some other engine's allow multiple datasets (simphony-lammps-md).

[tuopuu]

I don't know what the general requirement is... currently,we have simphony-jyulb(and I think simphony-openfoam) allowing one to have a single dataset while some other engine's allow multiple datasets (simphony-lammps-md).

Ok, if JyuLB is not the only wrapper allowing just one dataset, then I guess we can let it be that way for now. However, it would be nice if test_iter_datasets test is modified so that it checks if the wrapper can hold more than one and adjusts the test accordingly. Then possibly more wrappers, also from any future ones, can benefit from the test template.

[tuopuu]

I think it's important to abbreviate here that there's quite a lot of differences in the lattice container behaviors and how the test template works:

1. The default initiated Lattice returns LatticeNodes with an empty DataContainer, because we wanted these containers to use minimal amount memory
2. JYULatticeProxy has always 4 CUBA keys defined for its LatticeNodes
3. Currently abc_check_lattice only tests LatticeNodes against DataContainer() which is always empty.
4. If we change abc_check_lattice template so that LatticeNodes are tested against say,

create_data_container(restrict=self.supported_cuba())

then JYULatticeProxy will survive the test given that its supported_cuba is defined correctly. In this case the DataContainer would have the correct keys defined.
5. However, now the Lattice fill fail this test, because it's supported_cuba is set(CUBA), and it still returns empty LatticeNodes which is different what the template expects: LatticeNodes with all keys defined.

The solution to this might be a simple one, but like always, it's not so easy to find. :)

[kemattil]

1. We could also return with default values for the supported CUBA keys, but where would these default values be defined (perhaps implicitly by the wrapper?)?
2. This depends on the wrapper (in general, proxy lattices are specific to a wrapper); for example, multicomponent LB wrappers will support more CUBA keys for node data.

[tuopuu]

(perhaps implicitly by the wrapper?)?

Or maybe even explicitly, so that the user could ask supported_cuba from the wrapper. Then they would be open to test templates as well.

multicomponent LB wrappers will support more CUBA keys for node data.

This is a good point. We should try to solve this problem as generally as necessary just to avoid problems in future when new wrappers are implemented in Simphony.

[nathanfranklin]

(perhaps implicitly by the wrapper?)?

Or maybe even explicitly, so that the user could ask supported_cuba from the wrapper. Then they would be open to test templates as well.

This has been proposed at https://publicwiki-01.fraunhofer.de/SimPhoNy-Project/index.php/Supported_CUBA_keys_for_container

[kemattil]

I was referring to the actual data values (e.g. a default value for density)

[nathanfranklin]

  (perhaps implicitly by the wrapper?)?

Or maybe even explicitly, so that the user could ask supported_cuba from the wrapper. Then they would be open to test templates as well.

This has been proposed at https://publicwiki-01.fraunhofer.de/SimPhoNy-Project/index.php/Supported_CUBA_keys_for_container

I was referring to the actual data values (e.g. a default value for density)

Okay. I understand now. For the test templates, I totally agree that there the supported_cuba method (that the concrete tests provide) providing default values would be very helpful. For the wrapper, I am not so certain about that wrapper providing these default values.

[tuopuu]

I was referring to the actual data values

Ok, misunderstood that. Maybe wrapper should not provide these.

This has been proposed at https://publicwiki-01.fraunhofer.de/SimPhoNy-Project/index.php/Supported_CUBA_keys_for_container

Thanks for the info @nathanfranklin.I'll take a look at it.

[tuopuu]

Thanks for the info @nathanfranklin.I'll take a look at it.

Ok, I think this proposition is good and should be implemented as soon as possible, so that we can start using it for example for this issue.

Do you know if this proposal is approved or not by the SSB?

[nathanfranklin]

Do you know if this proposal is approved or not by the SSB?

No I don't but all the feedback has been positive so we should move that to the dev-team. But I wouldn't help this issue as it just affects the wrapper.

I think what you said earlier about "the ABCheckLattice template is modified so that it does not expect empty nodes returned from get_node, and methods alike" is the way to go.

This boils down to CheckLatticeNodeOperations requiring that:

• what the original node-data (i.e. CUBA + values) looks like for testing get_node, iter_nodes.
• what does an allowed updated node-data (i.e. CUBA + different values) look like which would be used for testing 'update_nodes'