-
Notifications
You must be signed in to change notification settings - Fork 57
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
adding namespaces #325
adding namespaces #325
Conversation
Is this already fully functional? It seems that there are still a few bits missing, e.g. there are no new files generated yet. |
…ruct in main namespace only
It is not fully functional as of yet. Yes adding the parent namespace is left. There was name conflicts occuring when using multiple modules at the same time, i.e. Main.x and Y.x would cause a name conflict and result in an error in the previous commit, so It took me some time to figure out how to avoid that. |
It looks like things start to work again. Very nice. I just had a quick look at the unittests. Could you add one of the datatypes that use namespaces there as well? E.g. Lines 139 to 153 in a8efda9
|
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.
This looks quite good already I think. I have a few minor comments / questions inline.
{% macro julia_parameters(params) %} | ||
{%- if params_jl -%}{ | ||
{%- set comma = joiner(',') -%} | ||
{%- for par in params -%} | ||
{{ comma() }}Main.{{ par }}Struct | ||
{%- endfor -%} | ||
}{%- endif -%} | ||
{% endmacro %} |
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.
If I remember correctly a very similar macro already exists in the MutableStruct.jl.jinja2
template(?). Is it possible to combine both macros into one and place it into a macros/julia.jinja2
file and simply import that here? It would probably have to take more arguments to deal with the different pre- and suffixes.
@@ -0,0 +1,2 @@ | |||
include("{{ class.bare_type }}.jl") | |||
{{ class.bare_type }}Collection = Vector{ {{ class.bare_type }}{% if params_jl %}{ {% for relation in params_jl %} {{ relation }}, {% endfor %} }{% endif %} } |
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.
{{ class.bare_type }}Collection = Vector{ {{ class.bare_type }}{% if params_jl %}{ {% for relation in params_jl %} {{ relation }}, {% endfor %} }{% endif %} } | |
{{ class.bare_type }}Collection = Vector{ {{ class.bare_type }}{% if params_jl %}{ {{ params_jl | join(', ') }} }{% endif %} } |
For consistency with how it is done in the constructors
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.
Collection.jl.jinja2
was a mistake.
}{%- endif -%} | ||
{% endmacro %} | ||
|
||
{{ class.bare_type }}Collection = Vector{ Main.{{ class.bare_type }}Struct{{ julia_parameters(params_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.
What is the purpose of this, since there is also a Collection.jl.jinja2
template now?
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.
Collection.jl.jinja2
was a mistake.
tests/unittest.jl
Outdated
include("datamodel/ExampleMC.jl") | ||
using .ExampleMCModule: ExampleMC |
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 it possible to bring the ExampleMCCollection
here via using .ExampleMCModule: ExampleMC, ExampleMCCollection
?
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.
simply doing include("ExampleMCCollection")
works as of now which also makes the ExampleMC. We can call both ExampleMCCollection( ) and ExampleMC( ) directly.
Since we are bringing the Module out from its namespace anyway in the Collection, we do not explicitly need to include ExampleMC. so L1 and L2 can be removed.
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Co-authored-by: Thomas Madlener <thomas.madlener@desy.de>
Hi @soumilbaldota, this looks like it solves things for the example datalayout of podio. I briefly tried to run it on the edm4hep definition and I have run into the following problem: julia> include("edm4hep.jl")
WARNING: replacing module Vector3fModule.
WARNING: replacing module Vector3fModule.
[... more of those ...]
WARNING: ignoring conflicting import of Vector3dModule.Vector3d into Main
ERROR: LoadError: UndefVarError: ObjectID not defined
Stacktrace:
[1] top-level scope
@ ~/work/EDM4hep/edm4hep/edm4hep/TrackerHitStruct.jl:5
[2] include(fname::String)
@ Base.MainInclude ./client.jl:451
[3] top-level scope
@ ~/work/EDM4hep/edm4hep/edm4hep/TrackerHit.jl:1
[4] include(fname::String)
@ Base.MainInclude ./client.jl:451
[5] top-level scope
@ ~/work/EDM4hep/edm4hep/edm4hep/edm4hep.jl:17
[6] include(fname::String)
@ Base.MainInclude ./client.jl:451
[7] top-level scope
@ REPL[1]:1
in expression starting at /home/tmadlener/work/EDM4hep/edm4hep/edm4hep/TrackerHitStruct.jl:5
in expression starting at /home/tmadlener/work/EDM4hep/edm4hep/edm4hep/TrackerHit.jl:1
in expression starting at /home/tmadlener/work/EDM4hep/edm4hep/edm4hep/edm4hep.jl:17 From a glance at the stacktrace it looks as if some of the necessary includes are missing for edm4hep. Could you have a look at that and try to find what the problem is with this? If you want to do a standalone generation of the edm4hep datamodel, the invocation of the code generator is: python podio_class_generator.py <path/to/>edm4hep.yaml <path/to/output/> edm4hep ROOT |
Just fixed it. |
I tried again with edm4hep, and it looks like the collections are not part of the module that is generated? Is that possible? julia> include("edm4hep.jl")
julia> edm4hep.MCParticle()
MCParticleStruct{MCParticleStruct}(0, 0, 0, 0.0f0, 0.0f0, 0.0, Vector3dStruct(0.0, 0.0, 0.0), Vector3dStruct(0.0, 0.0, 0.0), Vector3fStruct(0.0f0, 0.0f0, 0.0f0), Vector3fStruct(0.0f0, 0.0f0, 0.0f0), Vector3fStruct(0.0f0, 0.0f0, 0.0f0), Vector2iStruct(0, 0), MCParticleStruct[], MCParticleStruct[])
julia> edm4hep.MCParticleCollection()
ERROR: UndefVarError: MCParticleCollection not defined
Stacktrace:
[1] getproperty(x::Module, f::Symbol)
@ Base ./Base.jl:35
[2] top-level scope
@ REPL[5]:1 Would it be possible to place the collections into the modules as well? |
Hi @tmadlener, add Collection to namespaces fixes the problem now. |
Very nice. Works for me now as intended. If you have any very clever idea of how to get rid of the multiple imports of the same components into the same module we can put that into a separate pull request. |
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
* grouping code into modules * avoiding name conflicts between Structs and Constructor, including struct in main namespace only * adding parent namespace code generation * add Collection to namespaces
BEGINRELEASENOTES
ENDRELEASENOTES