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

Add unique ID to Node #153

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

joscao
Copy link
Contributor

@joscao joscao commented Sep 14, 2023

Adds a uid to the Node baseclass via the uuid module. Refer Issue #123

Current implementation utilizes uuid4() which creates a

universally unique identifier that is generated using random numbers, (https://www.uuidgenerator.net/version4)

e.g. UUID('d5181261-c06e-4f68-82ed-8f8d4592511b')

Two implementation questions arise:

  1. Is this UUID type for loki's case a reasonable choice or would something simpler, i.e. a generator of incrementing integers, be enough?
  2. Is there a reason that the Comment class was not calling the Node post init function? As far as git blame goes there once was a call to the base class init_ but it was removed @mlange05 (d6fe4e1).

Current tests are:

  • iterating over all fortran files in project, parsing them to ir and then walking over all nodes and checking if they have a uuid and if it is unique (by collecting a list of used uuids)
  • update/ clone tests: if update retains uid and clone creates new uid: for that all Node types are collected with dummy constructor arguments and checked if behavior is as described above

See Issue ecmwf-ifs#123, copy follows:

Currently, the only mechanism for differentiating two string-identical
but distinct nodes is the .source property. However this property is only
instantiated for parsed nodes; if two identical but distinct nodes are
generated in a transformation there is no method (apart from the object
id) to tell them apart.

The above could be remedied by adding a uid attribute to the Node class.
The uid property would be generated every time a node is created. This
means that even copies of the same node would have different values of
uid, unless overridden via a constructor argument.
I see no reason why Comments node wouldnt call the __post_init__ in
their __post_init__ like all other nodes do.
@codecov
Copy link

codecov bot commented Sep 29, 2023

Codecov Report

Merging #153 (f479128) into main (da56a26) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #153   +/-   ##
=======================================
  Coverage   92.13%   92.13%           
=======================================
  Files          90       90           
  Lines       16714    16717    +3     
=======================================
+ Hits        15399    15402    +3     
  Misses       1315     1315           
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.07% <100.00%> (+<0.01%) ⬆️
transformations 91.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
loki/ir.py 92.76% <100.00%> (+0.03%) ⬆️

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.

1 participant