search

simphony / simphony-jyulb

SimPhoNy Wrapper for the JYU-LB code.
0 stars 0 forks source link

Fix lattice testing #29

Closed nathanfranklin closed 8 years ago

nathanfranklin commented 8 years ago

Addresses problem of using ABCCheckLattice mentioned in simphony/simphony-common#216

nathanfranklin commented 8 years ago

@itziakos , @kemattil , now test_jyu_lattice_proxy.py runs.

nathanfranklin commented 8 years ago

JYULatticeProxy currently requires that all required CUBA appear in the external_node_data parameter at object initialization as what CUBA appear on each node.data cannot be changed later. Should this be documented somewhere? Probably in docstr JYULatticeProxy and/or add_dataset ..and as well as manual of simphony-jyulb. If so, let's make an issue to track the need for documentation

tuopuu commented 8 years ago

This looks good, I'll merge it to my work branch. Thanks @nathanfranklin.

tuopuu commented 8 years ago

We probably should update the documentation for all proxy objects if there are any restrictions to supported CUBA keys and other things. A separate issue would make sense.

kemattil commented 8 years ago

Please note that there are already two proxy lattices: 1. class JYULatticeProxy for the file-io wrapper and 2. class ProxyLattice for the internal wrapper (the class names should probably be harmonized). They operate differently, e.g. the proxy lattice for the internal wrapper assumes fixed CUBA keys (not configurable by the client) for the node data (these are clearly documented). Probably the class JYULatticeProxy could adopt a similar approach?

In general the proxies are wrapper dependent and hence there will be more proxy lattices. Therefore the comment by @nathanfranklin is very relevant and we should think whether there are some guidelines the proxy implementations could follow ...

tuopuu commented 8 years ago

(the class names should probably be harmonized)

That is on my to-do list, but I don't have good candidates yet. The naming should somehow remind of the datastructure (Lattice), the fact that it's not a real lattice, so that certain ABCLattice functionality may not be available (Proxy), and the wrapper itself (e.g. JyuLBInternal). But it does become quite long already: JyuLBInternalProxyLattice...

kemattil commented 8 years ago

That is the problem. Moreover, the "internal" is not enough to distinguish the proxies because we will have, ultimately, JyuLBInternalIsothermalProxyLattice, JyuLBInternalIsothermalMultiphaseProxyLattice, etc.

At the moment, I don't have good ideas how to name them ... but I dropped the "internal" and "fileio" because the implementations are in different packages/modules(?).