OpenEnergyTools / scl-lib

5 stars 4 forks source link

Fcda ext ref check #23

Closed JakobVogelsang closed 1 year ago

JakobVogelsang commented 1 year ago

There is no issue opened for it, but I am very sure that there are used cases to export it. I remember that we disable list elements when subscription is not possible. To determine this, you might need this function.

I would love you having a look @danyill , mainly it would interest me how performant this is.

This PR also has a little bug fix included. I failed with an extRef like so <ExtRef pDO="someInvalidDO" pDA="stVal" />

danyill commented 1 year ago

The difficulty in using this as part of a UI component is the need to query this information constantly (for disabling of disallowed connections).

I have a function like this:

private getFcdaInfo(fcdaElement: Element): fcdaInfo {
    const id = `${identity(fcdaElement)}`;
    if (!this.fcdaInfo.has(id)) {
      const spec = fcdaSpecification(fcdaElement);
      const desc = getFcdaInstDesc(fcdaElement, true);
      this.fcdaInfo.set(id, { spec, desc });
    }
    return this.fcdaInfo.get(id)!;
  }

And then I delete this info if an FCDA's cached information were to change (for instance from a menu plugin). So this means that once the base type information is recorded for the first time it's a constant time lookup.

If we don't do something like this, I think render functions will call this as the selection is changed and performance will be affected.

I suspect the ExtRef function will be much more performant with the new approach (haven't tested yet) but I very much doubt that recursing through all the datatypes will provide acceptable performant when we list every FCDA in the file in the subscriber plugin without a "caching layer".

Could we declare a baseType and pass this information in?

type baseDataType = {
    cdc: string,
    bType?: string 
}

Happy to discuss.

JakobVogelsang commented 1 year ago

Thank you, @danyill, for your review. I am with you that the function is not performant enough in the used case you have described. And more important, this is a very commonly used case. That lead to the conclusion that we need to tackle this. Is would propose to create a nsd.json lookup as we do with the ExtRef element. I think though that this need to have some more refinement. I would furthermore propose to do that as an extra refactor rather that doing it right away. I think you can still use it with the "cash" solution you have described.

The second point you have mentioned, the extra type. I do disagree with you on this point. We must not forget that an extra type add more complexity as well, as it has to be looked up. This is a trade-off of course. With very complex types that are used multiple times, an extra type definition does make sense. In this case, I feel the definition is still simple enough. @danyill what would you say?

danyill commented 1 year ago

The second point you have mentioned, the extra type. I do disagree with you on this point. We must not forget that an extra type add more complexity as well, as it has to be looked up. This is a trade-off of course. With very complex types that are used multiple times, an extra type definition does make sense. In this case, I feel the definition is still simple enough. @danyill what would you say?

Seems fair.

I'm happy to wait for a refactor as well.