Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3 create interfaces for frames #5

Merged
merged 13 commits into from
Nov 11, 2023
Merged

Conversation

andreapasquale94
Copy link
Member

@andreapasquale94 andreapasquale94 commented Oct 25, 2023

  • Adding interface macro
  • Basic model interfaces
  • Frames interfaces
  • Graphs interfaces

@MicheleCeresoli
Copy link
Member

  • There are some issues with the @interface function, it throws an UndefVarError because it does not recognise the NotImplementedError contained in their default behaviour.

  • The function load(provider::Type{<:AbstractEphemerisProvider}, files) already provides an implementation, will the @interface macro override that?

  • I just don't agree on adding the dependency of SMDGraphs at this point. Why can't we just declare in this pull request the AbstractGraphNode type in the interfaces and later make SMDGraphs inherit from it? We would just be introducing a dependency that we would have to fix with another pull request.

  • I don't really see the point on having AbstractFrameGraphPointNode and AbstractFrameGraphAxisNode in the interfaces. In FrameTransformations FrameAxesNode and FramePointNode are completely internal types that only depend on the way the frame system has been designed and thus should not require an interface definition. On the other hand, if they are intended to provide an abstract supertype for AbstractFramePoint and AbstractFrameAxes, I believe that we should stick with integer numbers (i.e., the NAIF IDs) to identify point and axes at the interface level.

  • Why is the AbstractFrameGraphModel parametric in T?

  • I would remove the parametric type for the time argument of all the transformation functions. It should be left as a generic Number field as currently implemented in FrameTransformations. I'm also a bit concerned about documenting those functions. It could mean that everytime the user looks at the documentation, it is always going to read that before the actual documentation of the function that is currently implemented in FrameTransformations. Any thoughts?

@andreapasquale94
Copy link
Member Author

andreapasquale94 commented Nov 11, 2023

Everything was solved with the previous commits.

The function load(provider::Type{<:AbstractEphemerisProvider}, files) already provides an implementation, will the @interface macro override that?

No, the implementation will be working correctly.

I just don't agree on adding the dependency of SMDGraphs at this point. Why can't we just declare in this pull request the AbstractGraphNode type in the interfaces and later make SMDGraphs inherit from it? We would just be introducing a dependency that we would have to fix with another pull request.

Agreed, dependency dropped.

I don't really see the point on having AbstractFrameGraphPointNode and AbstractFrameGraphAxisNode in the interfaces. In FrameTransformations FrameAxesNode and FramePointNode are completely internal types that only depend on the way the frame system has been designed and thus should not require an interface definition. On the other hand, if they are intended to provide an abstract supertype for AbstractFramePoint and AbstractFrameAxes, I believe that we should stick with integer numbers (i.e., the NAIF IDs) to identify point and axes at the interface level.

Agreed, AbstractFrameGraphPointNode, and AbstractFrameGraphAxisNode were removed.

Why is the AbstractFrameGraphModel parametric in T?

This is to provide eventual compatibility with Graphs.jl. I would keep it like that.

I would remove the parametric type for the time argument of all the transformation functions. It should be left as a generic Number field as currently implemented in FrameTransformations. I'm also a bit concerned about documenting those functions. It could mean that every time the user looks at the documentation, it is always going to read that before the actual documentation of the function that is currently implemented in FrameTransformations. Any thoughts?

Agreed, time is left generic as Number.

The documentation of the packages implementing the interfaces will not see the documentation of the interfaces, don't see the point here.

@MicheleCeresoli MicheleCeresoli merged commit aa53e1c into dev Nov 11, 2023
12 checks passed
@MicheleCeresoli MicheleCeresoli deleted the 3-create-interfaces-for-frames branch November 11, 2023 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix custom erros docstring Re-work interfaces towards Graphs/JSMDGraphs Create interfaces for frames
2 participants