Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Add known forth for ATLAS #1282
base: main
Are you sure you want to change the base?
feat: Add known forth for ATLAS #1282
Changes from 9 commits
3e3e716
d4acbdc
d27cc71
5a0456c
d23c711
a569eec
e9f7dad
f2246b7
eb3b970
3c3ca6f
3b26ac4
7701596
350f71e
13c7ffe
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpivarski i'm a bit uncomfortable with the piece of code i added here. The
_form
attribute already existed before, but was never directly returned here. The tests seem to pass, but do you know if there are potential problems that may be caused by directly returning_form
if it is not None?One thing that i had to do (and also don't quite feel comfortable with) was to convert the form from a dict representation which seems to happen sometimes (but i'm a bit unsure where and why not always).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still trying to understand your first paragraph, but about the second paragraph: there are three ways that a Form can be represented,
ak.forms.Form
object, which is immutable and therefore needs to be entirely known or at least built from the leaves up to the rootThus, they have decreasing levels of safety and we prefer the safer ones when possible. The problem is that the process of discovering the type involves starting at the root and walking down toward the leaves, filling in a ListOffsetArray's content if we see a non-empty example of it. So the ideal,
ak.forms.Form
, isn't possible and the Form starts its life as a Python dict. When it gets converted into an object, that's final. The Forth is fully discovered at that point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i now realized that the
self._form
attribute will always be in the Python dict form (either from generated or known forth code). So at the place where it is returned inAsObjects.awkward_form(...)
it has to be converted to the finalak.forms.Form
object. In principle the form from the known forth path could have been provided directly asak.forms.Form
, but to be consistent with the one created from the ForthGenerator it is now also a dict.