Closed budschi closed 6 months ago
we have some room for improvement:
@budschi we can start with these points, together to yours:
Hi @hampusnasstrom while fixing something around I saw basesections.py line 1333:
archive.workflow2.outputs = [
Link(name=result.name, section=result) for result in self.results
]
I got an Attribute Error because my class was an ArchiveSection instead of MeasurementResult and was hence missing the name quantity.
I propose to change it as follows, e. g., performing a very general check of the class before doing whatever to it.
If you have some MR open in gitlab, would you agree to add it? Otherwise I can open a new MR
for result in self.results:
if isinstance(result, MeasurementResult):
archive.workflow2.outputs.append(Link(name=result.name, section=result))
else:
logger.error(f"{result} is not a MeasurementResult inheriting class.")
Hrmm, shouldn't result
always be MeasurementResult
here? How did you manage to change it into something else?
results = SubSection(
section_def=MeasurementResult,
description='''
The result of the measurement.
''',
repeats=True,
)
In this case, the code was written before the existence of the class. I propose this as a better error messages in case of errors when developing new code too
I see. The problem is then that we would have to check all quantities and subsections to make sure they are of the specified type. I'm not sure if that is really good Python practice, or what do you think?
I don't know which is the very best practice, I just though we want to be sure in this case that the class is the required one because we're gonna fish more quantities for further processing that may not give rise to errors when missing or stick more normalizers to that basesection and we want to be sure are run
Sure, I'm just saying that this is only one out of very many times in basesections.py
that we assume that a self.something
is of the type we define. If we perform the check this time we should also add this check to all those other instances which might not be scalable.
going further with the inheritance, as we do progressively more, makes tedious to go and check all the inheritance tree to sketch where the missing attribute originates from, especially when we jump importing from nomad or from other plugins, so it's convenient for development to facilitate and tell with an error (or warning) raise what is wrong, there are not so many normalizers so one could either:
I think a class check is not harmful, is it?
@TLCFEM what do you think? Should we leave back the class check as a general strategy of coding or not?
@TLCFEM what do you think? Should we leave back the class check as a general strategy of coding or not?
It can be done during assignment. I am not sure if it has been done or not (likely not).
At your end, I do not think you need to implement this. It shall be handled in the general, area-independent metainfo.py
.
If I remember correctly, we had a related discussion before. However, I cannot recall whether the fact that this check is not performed is a feature/bug. It could be intended to provide extra flexibility as in some certain cases, maybe one want to append a different section to the list.
So it may be risky to force such a check at the global level as it may break things. But I guess it is possible to introduce a flag in the definition such that
my_subsection = SubSection(section=target_section.m_def, validate_section=True)
When populating data, the check can be performed if validate_section
is turned on. By such, at least in your area, you only need to add a flag to all related definitions, there is no need to manually check all usages, which is neither scalable nor visually appealing.
Thanks for your comments!
I summarize the points left and I will open a new issue for them:
At the moment the LayTec EpiTT schema is developed for the MOVPE IKZ Ga2O3 Eperiment. However, it can be used for any exported LayTec EpiTT data in *.dat format.
Next steps:
Goal: Harmonizing the MOVPE IKZ Ga2O3 Eperiment schema and epiTT schema that would enable us to:
wafer
number, modify MOVPE experiment accordingly --> see #64@aalbino2 @ThomasTSC