Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Final step to split the registry from the parser #1595

Merged
merged 6 commits into from
Oct 22, 2022
Merged

Conversation

hgrecco
Copy link
Owner

@hgrecco hgrecco commented Sep 30, 2022

Overview:

  • All the code in facets is now independent of the definition textual format. In particular, defintions such as UnitDefinition, ContextDefinition an so on cannot be built directly from a string. (some functions are kept only temporarily to simplify but transition) Building Definition objects from string requires a parser that emits them.
  • The standart pint format is implemented in delegates/txt_parser using flexparser. Briefly each single line statement is mapped to a ParsedStatement class and each larger construct to a Block class.
  • The registry then has an adder function that takes a definition an incorporate it into the registry.

A few nice features of this approach:

  1. The Definition objects are standalone public objects, you can now build them in a programatic way and incorporate them to the registry using the define function that will dispatch to the correct adder:

    new_unit = UnitDefintion( ....)
    ureg.define(new_unit) # might be called add in the future

    No more being forced to use string definitions (but you can still use them if you want)

  2. Composition over inheritance. The Registry does not know how to parse a definition, but it delegates this to another class which can be changed. This makes it very easy to write another parser (faster, simpler) o try out a completely different file format.

  3. Error messages can be more meaningful.

Backwards incompatible changes

  • is_base parameter Definitions is not needed any more. It is now computed automatically leading to a leaner experience and also avoiding incompatible states

  • alias for dimensionality has been removed (for now at least) The only one defined was speed as an alias of velocity.

  • (Context|Group|System).from_lines and Definition.from string have been rewritten in terms of the new parser. But will be likely removed in the future

  • Changing non_int_type is not possible after registry has been created

  • load_definition raises FileNotFoundError instead of a generic exception if the file was not found

  • the string representation of several definitions is now not so user friendly terms of the new parser. But will be likely removed in the future

  • Changing non_int_type is not possible after registry has been created

  • load_definition raises FileNotFoundError instead of a generic exception if the file was not found

  • the string representation of several definitions is now not so user friendly. terms o

  • Closes # (insert issue number)

  • Executed pre-commit run --all-files with no errors

  • The change is fully covered by automated unit tests

  • Documented in docs/ as appropriate

  • Added an entry to the CHANGES file

Overview:
- All the code in facets is now independent of the definition textual format.
  In particular, defintions such as UnitDefinition, ContextDefinition an so on
  cannot be built directly from a string.
  (some functions are kept only temporarily to simplify but transition)
  Building Definition objects from string requires a parser that emits them.
- The standart pint format is implemented in delegates/txt_parser
  using flexparser. Briefly each single line statement is mapped to
  a ParsedStatement class and each larger construct to a Block class.
- The registry then has an adder function that takes a definition an
  incorporate it into the registry.

A few nice features of this approach:
1. The Definition objects are standalone public objects,
   you can now build them in a programatic way and incorporate
   them to the registry using the define function that will
   dispatch to the correct adder:

      >>> new_unit = UnitDefintion( ....)
      >>> ureg.define(new_unit)  # might be called add in the future

   No more being forced to use string definitions
   (but you can still use them if you want)

2. Composition over inheritance. The Registry does not know how to
   parse a definition, but it delegates this to another class which
   can be changed. This makes it very easy to write another parser
   (faster, simpler) o try out a completely different file format.

3. Error messages can be more meaningful.

Backwards incompatible changes
- is_base parameter Definitions is not needed any more. It is
  now computed automatically leading to a leaner experience and
  also avoiding incompatible states
- alias for dimensionality has been removed (for now at least)
  The only one defined was speed as an alias of velocity.
- (Context|Group|System).from_lines and Definition.from string
  have been rewritten in terms of the new parser.
  But will be likely removed in the future
- Changing non_int_type is not possible after registry has been
  created
- load_definition raises FileNotFoundError instead of a generic exception
  if the file was not found
- the string representation of several definitions is now
  not so user friendly terms of the new parser.
  But will be likely removed in the future
- Changing non_int_type is not possible after registry has been
  created
- load_definition raises FileNotFoundError instead of a generic exception
  if the file was not found
- the string representation of several definitions is now
  not so user friendly.
7fab34c1a0365060f0be10eae1cd119ab6c30a72
@hgrecco
Copy link
Owner Author

hgrecco commented Sep 30, 2022

As far as I can tell, the only error is due to Exceptions in dask not playing nice with being frozen instance classes. But we can get rid of that or try to fix it withe pickle version or using pickle related magic methods.

@hgrecco
Copy link
Owner Author

hgrecco commented Sep 30, 2022

As far as I can tell, the only error is due to Exceptions in dask not playing nice with being frozen instance classes. But we can get rid of that or try to fix it withe pickle version or using pickle related magic methods.

confirmed, all test are passe using frozen=False in exceptions.

@hgrecco
Copy link
Owner Author

hgrecco commented Oct 4, 2022

A colleague suggested changing txt_parser to txt_defparser, Parser to DefParser and so on. Parser will be kept only to parse the mathematical expressions (i.e. ParserHelper) in which there is no other format possible (nobody is going to use reverse polish notation I guess!)

@hgrecco
Copy link
Owner Author

hgrecco commented Oct 13, 2022

@jules-ch What do you think about merging this.

@hgrecco
Copy link
Owner Author

hgrecco commented Oct 18, 2022

I would like to merge this by the end of the week, so any other change does not diverge too much. Any opposition?

@hgrecco hgrecco merged commit c07fc9b into master Oct 22, 2022
@bors bors bot deleted the using_flexparser branch October 22, 2022 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant