Renaming the computation of number of module type such that it make sense

[MaxTousss]

Summary

Title say it all XD

Testing performed

Compilation, eye and vs-code "Replace all".

Related issues

n/a

Checklist before requesting a review

• I have performed a self-review of my code
• I have added docstrings/doxygen in line with the guidance in the developer guide
• The code builds and runs on my machine

Contribution Notes

This is only basic renaming, no need to bother everyone.

Please tick the following:

• The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in the ETSI software (the Work) under the terms and conditions of the Apache-2.0 License.

[nkarakatsanis]

Although I would agree that it makes more sense to replace "numberOfReplicatedModules" with "numberOfModuleTypes", it is not quite intuitiuve to declare: numberOfModuleTypes: size(replicatedModules)
I would prefer either:
numberOfReplicatedModules: size(replicatedModules)
or
numberOfModuleTypes: size(moduleTypes)
The latter suggests replacing all instances of "replicatedModules" with "moduleTypes"

[KrisThielemans]

The latter suggests replacing all instances of "replicatedModules" with "moduleTypes"

I think this would be a sensible thing to do, but maybe best left for later, and adding it to #121

@MaxTousss I suggest to keep the original computedField as well, such that we don't break backwards compatibility, and can just merge. good idea?

[KrisThielemans]

will need a search-and-replace of number_of_replicated_modules() in the Python files.

[MaxTousss]

Some context if other people come here and they haven't taken a read of the PETSIRD model: replicatedModules is a 2D list where the first dimension is for the type of module and the second dimension is for the different modules of a type.

Thus, I do not feel that renaming replicatedModules as moduleTypes would be correct: it does not encode "types of module". While replicatedModules might not be the most intuitive name out of context, it is, I believe, intuitive when you know what it is.

For the same reason, I believe that declaring numberOfModuleTypes: size(replicatedModules) is intuive in the context of a PETSIRD model.

As for backward compatibility, I think that we are still early on enough and that it is small enough that we should do these changes for our future self. Not saying that it must be now, but that it should not be dismissed for that reason.

Thus, I still believe that the proposed change is for the best.

@KrisThielemans I am confuse about "'keep the original computedField' (...) and can just merge (...)". The modification in this PR is, currently, only about changing the computedField, thus there is nothing to merge if we keep the original computedField, right? Also, good catch about the python case. I looked at the cpp helper and found it was not used, so assumed it was also not used in python. I would do the modification soon.

[MaxTousss]

Python and matlab call to the modified computed field were modified accordingly.
Also, made use of the computed field in the cpp helpers which did it 'by hand" before.

[KrisThielemans]

As for backward compatibility, I think that we are still early on enough and that it is small enough that we should do these changes for our future self. Not saying that it must be now, but that it should not be dismissed for that reason.

Thus, I still believe that the proposed change is for the best.

I certainly agree, but have had the feedback that people are annoyed to have to change their code so often...

@KrisThielemans I am confuse about "'keep the original computedField' (...) and can just merge (...)". T

Sorry, I meant, "keeping both the new and original computedField, ... and can just merge this PR (without affecting any use-cases), i.e.

ScannerGeometry: !record
  fields:
     # your stuff
  computedFields:
    # Warning: this function will be removed in the near future
    numberOfReplicatedModules: size(replicatedModules)
    numberOfModuleTypes: size(replicatedModules)

With that, we can merge the PR safely.

[MaxTousss]

I certainly agree, but have had the feedback that people are annoyed to have to change their code so often...

Hahahaha, it is true that I only work on PETSIRD twice per year so, in my eye, it is normal that a v0.x.y changes each time I use it XD

I do not mind to have both of them, but does Yardl has a "Deprecated" keyword for this kinds of situation? Like #deprecated that would auto-magic add a deprecated message when used? Might be a question for @casperdcl

[KrisThielemans]

Might be a question for @casperdcl

not really, This one is for @naegelejd. However, I'm 99.99% sure yardl doesn't have it yet. (You can read up on their version evolution plans on https://microsoft.github.io/yardl/cpp/evolution.html, but I certainly don't want to go there yet)

[KrisThielemans]

Meanwhile, we have C++ compilation errors

 error: 'struct petsird::ScannerGeometry' has no member named 'numberOfModuleTypes'; did you mean 'NumberOfModuleTypes'?

(The naming of the generated computed fields is a bit unpredictable, see microsoft/yardl#93)

[KrisThielemans]

@MaxTousss if you do these 2 things, I suggest I squash-merge.
Please use a comment like

# Warning: this function will be removed in the near future (TODO)

such that I won't forget to remove it later...

[MaxTousss]

The two things are done. Sorry about the cpp: I still use DevContainer for cpp compilation and thus testing the cpp part is done copying the modifications...

Squash away!