-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add parsers #90
Add parsers #90
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
I have some concerns that we're taking too roundabout a way to accomplish this, and that we'd be better off adding We can actually already almost do this. E.g., at the class level and with a macro: from pyiron_workflow import as_macro_node
from semantikon.typing import u
cookie_count = u(int, units="cookies", triple=("a", "b", "c"))
@as_macro_node
def SomeMacro(
self,
in1: cookie_count,
in2: float
) -> tuple[cookie_count, float]:
self.int_out = 2 * in1
self.float_out = in1 + in2
return self.int_out, self.float_out
relevant_ontological_info = SomeMacro.preview_io()
relevant_ontological_info.update({"node": SomeMacro.__name__})
relevant_ontological_info Gives {'inputs': {'in1': (int, NOT_DATA), 'in2': (float, NOT_DATA)},
'outputs': {'int_out': int, 'float_out': float},
'node': 'SomeMacro'} On the one hand, I'm super happy that {'inputs': {'in1': (u(int, units="cookies", triple=("a", "b", "c"), NOT_DATA), 'in2': (float, NOT_DATA)},
'outputs': {'int_out': u(int, units="cookies", triple=("a", "b", "c"), 'float_out': float},
'node': 'SomeMacro'} For a node-recommendation system, it's critical that we be able to work on the class-level like this. For walking an actual graph instance and looking at transitive ontological requirements, we'd need the In this way, I would imagine that |
Do I understand correctly that you are talking about the implementation and not the overall functionality of When it comes to what does what, I can definitely see your point. I just feel I'm not really entitled to say whether it should be included in |
Yes, I agree we want a function that parses a node (or a node class, which differs from this PR) and returns a dictionary identical or very similar to what you have here. I just think it should live in
This is actually why I would push towards It's actually a very similar deal for Jan's more generic workflow representation, and these are the two dictionaries I think we want We can and should keep {
"type": "some_node_package.AllergenDetector",
"label": "this_label_also_appears_in_the_graph_dictionary", # Optional
"inputs": {
"x": u(int, units="cookies", uri="baking.Cookie"),
},
"outputs": {
"chocolate_mass": u(float, units="g", triple=("chocolate", "isPropertyOf", "inputs.x"), uri="baking.Chocolate",
"has_nuts": u(bool, triple=("hasNuts", "isPropertyOf", "inputs.x")),
}
} Don't take my triples too seriously here -- I think my understanding of this concept still lags yours and Sarath's significantly. Same with what I shoved in for In the same way, we could have a graph dictionary like {
"nodes": {
"oven": some_node_package.MakeCookie,
"checker": some_node_package.AllergenDetector
},
"edges": (
("oven.outputs.cookie", "checker.inputs.x"),
)
} And again, this is super easy with In this way, we can imagine taking some But from what I see now, we should be able to build up |
I think we are on the same page on this topic. I would even go a step further and say there isn't such a thing as
There's a possibility that we would have to develop our own reasoner, but that's still very different from what I wrote for this PR, so currently there's no space for pyiron_ontology in my plan. Maybe: Is it possible that you are thinking I'm only implementing what I presented before? If that's the case, no, that's not what I'm trying to do. I'm going more in the direction of how to represent a workflow (cf. this issue). The problem with the prototype that I had presented was that there is no connection between functions and inputs/outputs, even though according to my understanding, there should be intransitive connections for |
👍 Yep, at least insofar as I understand the topic 😂
I guess the role I see for
Yes, 💯. My current vision is that the "interpreter" combines the graph connectivity and the annotations to come up with raw (but perhaps rich and complex) ontological objects, and that we can then pass these to an off-the-shell reasoner that only knows about the ontology itself in order to answer questions about whether or not one object is "as or more specific than" the other object from an ontological perspective.
Hmm, I start to get a little confused here. My own grasp of what I mean by "(in)transitive" is, honestly, pretty rough, so I can't be at all certain we're using it the same way. I certainly don't disagree with anything in this paragraph though! I guess I feel pretty confident at this stage about how we can represent (exclusively DAG) workflows in a simple dictionary-like way, and the "intransitive"(?) ontological connections between input and output across a "node" in a simple dictionary-like way, and pretty un-confident in how we can bring these together in an "interpretation" to achieve "transitive" updates to resulting "node" I/O ontological hints. In light of that lack of confidence, I look forward to hearing about your ideas! There is no agenda yet for Monday, so maybe there is space to discuss it at the regular meeting? Otherwise let's find another time for you to show off the ideas! |
Sorry actually transitive/intransitive was not really the focus of what I wanted to say. What I wanted to say was the knowledge graph should contain not only data, but also the workflow nodes, and that's what I wanted to achieve in this PR. Don't know how I ended up with the discussion with transitive/intransitive. Anyway I'm pretty confident that it's the right move, especially after I realized that @jan-janssen had the same idea independently. |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
I start adding
pyiron_workflow
parsers in order to take ontology into account, usingsemantikon
.