Added ThinFilmCombinatorialSample with properties #111
Conversation
|
@RoteKekse and @tunold I now added the @ka-sarthak could you review it from Area A side? @Pepe-Marquez just FYI :) |
There was a problem hiding this comment.
Looks great to me. The schemas are well-defined. I added some suggestions related to quantity descriptions and units.
Also, you are defining the units for wavelength as nm. It's completely fine but if we want to be consistent with using SI for quantity definitions, we might want to change it to m.
| type=float, | ||
| description=""" | ||
| The peak area of the photoluminescence spectrum. | ||
| """, |
There was a problem hiding this comment.
Shouldn't units be defined for peak_area since we have quantities with units on at least x-axis (wavelength, or energy in general, I assume)?
There was a problem hiding this comment.
not sure what the correct unit is, if it is wavelength over intensity, i think leaving it like this is ok
Co-authored-by: Sarthak Kapoor <[email protected]>
Co-authored-by: Sarthak Kapoor <[email protected]>
|
@RoteKekse @ka-sarthak why is this merged in main ? ? |
Why not? Do you see any fundamental problems? |
|
This will be revised anyhow next week as we are testing the combo classes now in the HTEM database and other group plugins. |
|
it was not revised by everyone before merging.. it could live in one of that plugins i believe |
|
hampus made it and sarthak and me reviewed it |
|
and i then merged it |
|
This will be revised next week. |
hampusnasstrom
restored the
103-add-specialized-combinatorialsample-schema
branch
This reverts commit 212994c.
No description provided.