Improve and simplify reading and writing of tools on the example of print_array_elements #683

GernotMaier · 2023-11-16T14:20:09Z

Addresses partly #643 and is a step towards a more unified approach of input / output of tools. This PR deals mostly with unified output.

Functionality is added and tested mostly with the print_array_elements.py application, but partly also to derive_mirror_rnda.py (and minor changes to other applications to allow integration tests to run).
Goal is to first get input/output/metadata/validation working for one simple applications and then propagate this functionality to all other applications.

Conceptually new: introduced simtools/constant.py to put "hardwired" values like the paths to the schema files. Not sure if this is a best solution, the internet says it is acceptable.

Changes:

use MetaDataCollector and ModelDataWriter for reading / writing of metadata and data
improved metadata collection (many more fields are non-null now)
introduction of CONTEXT:ASSOCIATED_DATA field to list origin / input data
simplified interface to ModelDataWriter by adding a dump function. The following lines are need to write a consistently data and metadata:

   writer.ModelDataWriter.dump(
        args_dict=args_dict,
        metadata=MetadataCollector(args_dict=args_dict).top_level_meta,
        product_data=data_table
    )

this replaced the method to write the output table in layout_array:export_telescope list (as used in print_array_elements)
use input and input_meta as command line parameters for input data/metadata at least for the three applications mention above (this needs another thought, but I think we are a bit inconsistent with our command line between the application)
IMPORTANT replaced command line parameter use_plain_output_path by use_simtools_output_path, reversing the logic from before. Default is write into output path (if not set, the path ./simtools-output/ is used) without the additional directories for tools, date, etc. (this will have to be propagated into other applications)
read name of schema file for data validation from metadata (from PRODUCT:DATA:MODEL:URL) in metadata_collector:get_data_model_schema
simplified some logging messages
the widely used method collect_data_from_yaml_or_dict downloads a file to temp disk and returns a dict if the file name is a url
some adaption to the metadata examples files in tests/resources (e.g., correct schema variable URLs)
add validation of data before writing it to disk. This means that using ModelDataWriter.dump with validate_schema_file set used this schema file to validate the output, transform it if necessary to the units listed in the schemafile.

codecov · 2023-11-16T14:29:55Z

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (b169b3b) 81.93% compared to head (83a0356) 81.84%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
simtools/data_model/model_data_writer.py	33.33%	10 Missing ⚠️
simtools/data_model/validate_data.py	87.09%	4 Missing ⚠️
simtools/utils/general.py	89.65%	3 Missing ⚠️
simtools/layout/array_layout.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #683      +/-   ##
==========================================
- Coverage   81.93%   81.84%   -0.09%     
==========================================
  Files          40       41       +1     
  Lines        6128     6203      +75     
==========================================
+ Hits         5021     5077      +56     
- Misses       1107     1126      +19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Improved metadata filling and validation of output

VictorBarbosaMartins

Hi @GernotMaier. Sorry for the delay, this was a relatively long one and it took me some time to understand the code. I provide here a couple of comments and I hope it helps improving the code.
Thank you!

simtools/applications/generate_default_metadata.py

simtools/applications/make_regular_arrays.py

simtools/applications/print_array_elements.py

tests/unit_tests/data_model/test_metadata_model.py

VictorBarbosaMartins · 2023-11-30T13:32:03Z

tests/unit_tests/layout/test_layout_array.py

-        layout.export_telescope_list(crs_name="corsika")
+        _table = layout.export_telescope_list_table(crs_name="corsika")
+        _export_file = tmp_test_directory / "test_layout.ecsv"
+        _table.write(_export_file, format="ascii.ecsv", overwrite=True)


in order to test whether it was written, I suggest opening the file and checking whether the table is the same as before.

I think this is an integration test we need to do (and I have it on my list to implement).

Still on this PR or in another one?

No - we need to work on integration tests in our maintenance period (remember that we don't have good tests).

tests/unit_tests/utils/test_general.py

VictorBarbosaMartins

Thanks for considering the comments. I think we are good to go now if the tests pass.

simtools/applications/print_array_elements.py

simtools/configuration/commandline_parser.py

orelgueta

A few minor comments. Did not go through everything very carefully since @VictorBarbosaMartins is actually doing the review.

orelgueta · 2023-12-01T10:51:35Z

simtools/data_model/metadata_collector.py

+
+        if self.data_model_name:
+            self._logger.debug(f"Schema file from data model name: {self.data_model_name}")
+            return simtools.constants.SCHEMA_URL + self.data_model_name + ".schema.yml"


This can be shortened by changing the import to from simtools import constants or importing as a short name. Also, please use f-string instead of combining strings like that.

I consciously want to "simtools.constants", as modules called constants are quite common. Writing it like this improves readability (as one does not have to scroll up to see which import it is).

What is an advantage of f-strings to "+"?

OK, no problem to keep simtools.constants then, makes sense.

f-strings are significantly faster than adding strings and also for the purpose of consistency since I switched everything to f-string a while back.

OK, changed.

simtools/data_model/metadata_collector.py

simtools/data_model/metadata_model.py

simtools/schemas/metadata.schema.yml

simtools/utils/general.py

tests/resources/telescope_positions-North-utm.meta.yml

orelgueta · 2023-12-01T11:17:02Z

BTW, notice that there are some new lines uncovered by tests (codacy complains).

GernotMaier · 2023-12-01T11:43:30Z

@VictorBarbosaMartins , @orelgueta - thanks for looking into the code. Will merge after the tests.

GernotMaier added 2 commits November 16, 2023 13:24

metadata clarification

62dce94

simplified writing

42bb5f6

GernotMaier self-assigned this Nov 16, 2023

GernotMaier changed the base branch from main to metadata_refactoring November 16, 2023 14:22

GernotMaier added 2 commits November 16, 2023 16:49

tweaks for output

e425b7f

Merge branch 'metadata_refactoring' into derive_array_elements_output

b892ee4

GernotMaier changed the base branch from metadata_refactoring to main November 16, 2023 16:48

GernotMaier added 18 commits November 17, 2023 12:21

Merge branch 'main' into data_metadata_output

5484ab6

Merge branch 'main' into derive_array_elements_output

e2d8597

add reading from remote file

dc62bc9

test improvement

0a882a7

add name

332d5b5

improved reading of schema from file

c7f16f5

fix tests

fde507f

typo

dbdf481

add instrument description

792f491

Merge branch 'metadata_refactoring' into derive_array_elements_output

7c0145b

simplified writing

979b93f

table schema

981b892

move schema file getter

f2fcfc4

Merge branch 'main' into derive_array_elements_output

816b79b

Merge branch 'main' into data_metadata_output

87ee8f6

testing

4c4d2e8

remove multirow

17f5191

remove todo

2af0487

GernotMaier marked this pull request as ready for review November 21, 2023 14:22

GernotMaier requested a review from VictorBarbosaMartins November 21, 2023 14:22

GernotMaier added 2 commits November 23, 2023 07:58

Merge branch 'main' into output_validation

753baaa

Merge branch 'derive_array_elements_output' into output_validation

b5b3bb9

GernotMaier added 5 commits November 27, 2023 15:20

take differences in table column types for QTables into account

7905725

improve tests in data_validation

3bbd52f

tests and improved reading of schema file

65b59bb

Merge branch 'main' into derive_array_elements_output

d345939

Merge branch 'main' into output_validation

37754c0

GernotMaier mentioned this pull request Nov 27, 2023

Improved metadata filling and validation of output #691

Merged

GernotMaier added 6 commits November 28, 2023 10:06

missing docstring

83e6726

clarification on implementation of metadat model

7a98f52

fix test

ebde54b

user getpass.getuser

609ebc0

Merge branch 'derive_array_elements_output' into output_validation

835ab07

Merge pull request #691 from gammasim/output_validation

0c0837c

Improved metadata filling and validation of output

VictorBarbosaMartins suggested changes Nov 30, 2023

View reviewed changes

GernotMaier added 5 commits November 30, 2023 19:47

improvements following Victors comments

05d5cf6

Merge branch 'main' into derive_array_elements_output

9e9298e

consistent naming

942ae5c

fix unit tests and export files

320a314

merge fix

31a801d

VictorBarbosaMartins self-requested a review December 1, 2023 09:26

introduce for loop

3d5c30b

VictorBarbosaMartins approved these changes Dec 1, 2023

View reviewed changes

orelgueta reviewed Dec 1, 2023

View reviewed changes

simtools/applications/print_array_elements.py Show resolved Hide resolved

orelgueta reviewed Dec 1, 2023

View reviewed changes

simtools/configuration/commandline_parser.py Outdated Show resolved Hide resolved

use plain output

19c8f40

orelgueta reviewed Dec 1, 2023

View reviewed changes

change to f-string

83a0356

GernotMaier merged commit 97c22a5 into main Dec 1, 2023
7 of 8 checks passed

GernotMaier deleted the derive_array_elements_output branch December 1, 2023 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve and simplify reading and writing of tools on the example of print_array_elements #683

Improve and simplify reading and writing of tools on the example of print_array_elements #683

GernotMaier commented Nov 16, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading

VictorBarbosaMartins left a comment

VictorBarbosaMartins Nov 30, 2023

GernotMaier Nov 30, 2023

VictorBarbosaMartins Dec 1, 2023

GernotMaier Dec 1, 2023

VictorBarbosaMartins left a comment

orelgueta left a comment

orelgueta Dec 1, 2023

GernotMaier Dec 1, 2023

orelgueta Dec 1, 2023

GernotMaier Dec 1, 2023

orelgueta commented Dec 1, 2023

GernotMaier commented Dec 1, 2023

Improve and simplify reading and writing of tools on the example of print_array_elements #683

Improve and simplify reading and writing of tools on the example of print_array_elements #683

Conversation

GernotMaier commented Nov 16, 2023 • edited Loading

codecov bot commented Nov 16, 2023 • edited Loading

Codecov Report

VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

orelgueta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orelgueta commented Dec 1, 2023

GernotMaier commented Dec 1, 2023

GernotMaier commented Nov 16, 2023 •

edited

Loading

codecov bot commented Nov 16, 2023 •

edited

Loading