Skip to content

fmudesign: improvement ideas #774

Description

@tommyod

Some small comments:

  • sometimes strings are used for paths, sometimes pathlib.Path is used. we should use pathlib.Path for everything.
  • Fixed. OrderedDict is used. I don't think it is neede. New python versions have ordered dict anyway
  • We should raise if someone tries to pass in an option that is not valid, e.g. if someone has at typo and writes distributon_seeds

Some larger comments:

  • we suffer form the fact that the input is Excel: hard to work with binary files for documentation/version control/etc
  • we suffer from the fact that documentation is in another repo and relies on screenshots of Excel documents. as such it is very hard to update and requires manual work.
    • what if we had a subcommand fmudesign init that wrote Excel config files? then we should e.g. do fmudesign init design_input_correlations to create design_input_correlations.xlsx (this file would ship with the CLI), and the documentation/example files would be self-testing since we could run fmudesign init <file> && fmudesign <file> as part of the test suite.
    • we could write much more information in --help, it's not used much today at all.
  • not exactly sure what the responsiblity of reading the Excel file vs. setting up design is. for instance, where does the validation responsibility lie? this could be clarified.

Structure for file reading

Here is a sketch that would facilitate more thorough testing, separation of concerns and give us yaml config "for free". Yaml config (or toml or whatever, as long as it is text) seems like a very nice thing to have.

def parse_excel(filename) -> dict:
    """Parse an Excel config file and all dependent paths
    that it refers to. Does NOT do any validation except the
    bare minimum. Only responsible to read data and return a
    (possibly) messy or wrong dict-of-dicts with config."""
    pass

def parse_yaml(filename) -> dict:
    """Parse a YAML config file and all dependent paths
    that it refers to. No validation. Simply a call to a yaml
    library to read a config."""
    pass
    
def validate_config(config: dict) -> dict:
    """Validates a config file. Strips and converst strings,
    converts strings like '13.2' to floats, etc. Also validates
    that keys (option names) and values (option choices) are
    according to the specification of a valid config.
    Returns a clean config that is guaranteed to be correct."""
    # (1) If all cleaning is in this function, it is easy to test
    # because validation is not part of file reading!
    # (2) Also makes it trivial to have a YAML config, which is
    # easier to document, version control (for us and users), 
    # search, test, etc!
    pass


if filename.endswith(".xlsx"):
    config = parse_excel(filename)
elif filename.endswith((".yaml", ".yml")):
    config = parse_yaml(filename)
    
config = validate_config(filename)

DesignMatrix(config)

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

Status
Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions