-
Notifications
You must be signed in to change notification settings - Fork 60
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
Julia preprocessing #311
Julia preprocessing #311
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @soumilbaldota,
I have a few comments (some are inline) that are easier to handle right at the beginning as they are probably less work to address at the moment. What you have until now definitely goes into the right direction, I just would do some things slightly differently, and some things might not be necessary for the Julia part. However, you are currently working on this on certainly have more insights than me, so if I missed something let me know.
- The name of the template is currently
Julia.jl.jinja2
. I think here you have to find a slightly more descriptive name, because there might be more Julia template files and in principle the fact that it is a Julia template is already encoded in the.jl
suffix. A good name here should reflect that the template is used for a mutable Julia type/struct. - You have put the necessary preprocessing steps for the Julia template into the different existing preprocessing functions for the c++ code. Here I think it would make sense to define an entirely new
_preprocess_for_julia
(or similar) method, so that all the necessary steps can be found in one place. This makes it much easier to maintain in the future. - There are some things that look like they are there simply to reflect the c++ side of code generation. I am not entirely sure they are actually necessary. Making the julia preprocessing a separate method would make it easier to actually spot these and only handle what is actually necessary. (i.e. only add things here if they are needed in the templates)
python/podio_class_generator.py
Outdated
|
||
for relation in datatype['OneToManyRelations'] + datatype['OneToOneRelations']: | ||
if datatype['class'].bare_type != relation.bare_type: | ||
includes_cc.add(self._build_include(relation.bare_type + 'Collection')) | ||
includes_jl.add(self._build_include(relation.bare_type + 'Collection', julia=True)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Respectively can you use the generated files with this? I am asking because as far as I can see there is no XYZCollection.jl
that we generate here(?)
python/podio_class_generator.py
Outdated
@@ -359,29 +375,35 @@ def _preprocess_datatype(self, name, definition): | |||
data = deepcopy(definition) | |||
data['class'] = DataType(name) | |||
data['includes_data'] = self._get_member_includes(definition["Members"]) | |||
data['includes_data_jl'] = self._get_member_includes(definition["Members"], julia=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? In the templates I don't see that we use includes_data_jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see they are used in the templates now. Is there a reason why they have to go to a different list than the usual includes_jl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they can go in the same list. I will change it in the next commit.
python/podio_class_generator.py
Outdated
|
||
if datatype['VectorMembers']: | ||
includes_cc.add('#include <numeric>') | ||
|
||
datatype['includes_coll_cc'] = self._sort_includes(includes_cc) | ||
datatype['includes_coll_data'] = self._sort_includes(includes) | ||
datatype['includes_jl'].update(self._sort_includes(includes_jl)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the _sort_includes
work as expected with the Julia includes?
This looks pretty nice already. Is the generated code working? Can you add just a few tests using the example datamodel that comes with podio. I don't think it has to cover everything at this point, but e.g. something using the I have one question / comment. Maybe you have a stronger opinion here than me. From the current status it looks like the |
The generated code is working for them and I have run it with datalayout.yaml.
I think I can include this later also. Currently I am facing an issue that when I run class_generator with edm4hep.yaml. It generates the |
Ah that is an excellent double check to see if things are working as expected and whether we have overlooked something. Are forward declarations possible in Julia? Or do we have to think about a different solution to make this work with datatypes that refer to each other? |
They are not.
The below only works when both the structs are in the same file. mutable struct Vertex{T}
associatedParticle::Union{T ,Nothing}
end
mutable struct ReconstructedParticle{T}
startVertex::Union{Nothing, T }
end
function Vertex()
return Vertex{ReconstructedParticle}(
nothing,
)
end
function ReconstructedParticle()
return ReconstructedParticle{Vertex}(
nothing,
)
end |
I will have to think about it as well. Maybe there is an elegant solution using abstract types or something, but from the top of my head I am not entirely sure. Maybe @hegner has a clever idea for this. Another thing that we have not really dealt with in our prototype is the fact that the yaml definition allows to introduce a namespace (in c++), e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice approach and implementation for the templates. I have only a few minor comments inline and would also want to ask you whether you could add a couple of tests that use the example datamodel that comes with podio to see if things work with that. At least for the most important use cases like relations.
Could you also have a look at the CI failures and fix any obvious problems. Let me know if you need help with that.
/home/runner/work/podio/podio/tests/src/ExampleWithOneRelationSIOBlock.cc:13:29: error: unused parameter 'device' [-Werror,-Wunused-parameter] The above two errors are making the pre-commit fail but i dont know why. |
44e3615
to
e129173
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I just realized is that the current version is not working for me, because the generated Julia code is also passed through clang-format
, which messes with the formatting and makes the generated Julia code not work in the end.
You will have to add a check here
podio/python/podio_class_generator.py
Line 154 in a8efda9
if self.clang_format: |
that makes the Julia files not being run through clang-format
. Should be as simple as checking for the file-ending I hope.
I have added Collections in a separate template to Constructor because we are only making Collections of Datatypes and not Components and hence called fill_templates only in process_datatype() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent. Could you just add one or two tests that also use the collections? Obviously since they are Vector{T}
at the moment, there is probably nothing that breaks them, but if we need a more complicated implementation in the future we would at least immediately see where they break.
A question about the template names, resp. the produced filenames: I suppose you have to name it JuliaCollection
, because Collection
is already taken by the c++ side and a different suffix is not enough to distinguish them, right?
Co-authored-by: Thomas Madlener <[email protected]>
Co-authored-by: Thomas Madlener <[email protected]>
Co-authored-by: Thomas Madlener <[email protected]>
Co-authored-by: Thomas Madlener <[email protected]>
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
* adding pre-processing/"include" logic for julia * Add templates for Julia code
…473) * Parsing Julia types from c++ types in yaml description (#310) * Julia preprocessing (#311) * adding pre-processing/"include" logic for julia * Add templates for Julia code * Add Julia unittests to test suite (#322) * grouping code into modules * add documentation for Julia code generation (#329) * Added abstract types in default parameters and '-l' language argument * Added --upstream-edm code generation support * Created _sort_components_and_datatypes function to perform topological sort * Added abstract support for builtin Vector Members * Added conditional check for StaticArrays * Suggested Changes * Added ENABLE_JULIA toggle --------- Co-authored-by: soumilbaldota <[email protected]> Co-authored-by: Thomas Madlener <[email protected]>
BEGINRELEASENOTES
ENDRELEASENOTES