SED-ML / sed-ml

Simulation Experiment Description Markup Language (SED-ML)
http://sed-ml.org
5 stars 2 forks source link

Add subclasses of Variable? #221

Open luciansmith opened 8 months ago

luciansmith commented 8 months ago

Logan (@CodeByDrescher) had several good suggestions for the spec that I've now incorporated into the latest L1v5 draft. This one seemed like a potentially good idea, but I wasn't sure if it was worth the cost of actually changing the spec for. What do the rest of you think?


● Variable (Pages 15-19) is an absolute behemoth. With so many attributes to be as flexible as possible, the Variable has become a nebulous but integral part of SedML experiments. However, unless you happen to need a specific use case for a variable, you’re realistically not going to understand over half of Variable’s functionality (until you need it and wonder if SedML can do it and dig deep).

○ Heavily consider sub-typing Variable into multiple flavors of Variable (e.g. ReferenceVariable, PostProcessingVariable, etc.), in greater accordance with the Single-Responsibility-Principle. This would be similar to how there are technically multiple types of references, despite there being basically no syntactic difference. The difference is that we would permit a different naming of attributes for the specific variable flavor, because the attributes map to the god-Variable; accepting a different name binding is a small cost task. For backwards compatibility, absolutely have the Variable Object keep its current power, but still create Variable flavors with subset capability. By having more discrete, better described, single purpose variable sub-types, we can greatly improve the readability and understandability of SedML. Since most tools use SedML through a maintained library, this change would have limited ripple effects even if people did really start using the sub-types commonly.


The subclasses would (I think) basically be 'Variable, but with added validation rules'.

@fbergmann @nickerso @eagmon @matthiaskoenig

fbergmann commented 8 months ago

My thought would be to leave things as they are for now. While indeed there are many attributes on there, I'm not sure which ones to split up: we need targets to address things via xpath, symbols for things that are implied in the document that there are no targets to, the term to clarify the type needed, finally a reference to get things from. Maybe we could put the slicing into another class, but I'm not sure it would be worth the effort at this point.

Additionally, wouldnt the class hierarchy be the other way around ... that where basically, we'd have:

classDiagram
  SEDBase <|-- BasicVariable
  BasicVariable <|-- VariableWithSlicing
  VariableWithSlicing <|-- SedVariable

so it would not be that those other variables are variables with validation rules but rather bits of them, that could not be used where the others were used before (otherwise they'd have all the attributes and functions, that were just not valid to be used). Anyway I'd stay away from this for now and leave it to be settled in the eventual rewrite.

luciansmith commented 8 months ago

Leaving things the way they are is fine with me, but just to clarify, I would not arrange the hierarchy the way you have it there. I would make it something like:

Variable proposal

with 'Variable' being exactly the same as it is now. You'd only use a subclass if you wanted extra validation for the particular type of variable you were going for.

matthiaskoenig commented 8 months ago

@luciansmith I like what you propose here. I.e. the UML looks great and would make things clearer.

luciansmith commented 8 months ago

So that's one vote for the change and one vote against. Anyone else want to chime in? Our current editors are @nickerso @eagmon @jonrkarr and Tomas.

nickerso commented 8 months ago

While this might be a nice to have, I don't see the benefit outweighing the cost for making this change and getting it into tooling. And with the way SED-ML 2 is going, its not going to benefit from this kind of change either.

fbergmann commented 8 months ago

Variable proposal

with 'Variable' being exactly the same as it is now. You'd only use a subclass if you wanted extra validation for the particular type of variable you were going for.

This would also mean though, that each of those classes later on in your code have exactly the same methods, that you'd get suggested on autocompletion. Except that now the behavior would change sometimes:

createSomething: would create the something you wanted to create, at other times it wouldn't and instead produce an error. So i'm not sure i see the added value, only quite a lot of work.

eagmon commented 8 months ago

I agree with @nickerso -- this seems like a big lift that might not pay off. If we had a practical example SED-ML file that someone was trying to build, which demonstrates how Variable has been abused, then I would possibly reconsider. It does seem like we could keep backwards compatibility with the current Variable, so this wouldn't break too much. But it does add more complexity than I think is needed.