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

removal of dpath -- ready for review #680

Merged
merged 13 commits into from
Mar 24, 2024
Merged

removal of dpath -- ready for review #680

merged 13 commits into from
Mar 24, 2024

Conversation

dafnapension
Copy link
Collaborator

@dafnapension dafnapension commented Mar 18, 2024

Hi Elron and Yoav,
d_path is out of dict_utility, and of both requirement files where it showed.

I also cancelled the ban of process_every_value not allowed for operator RemoveValues (a ban laid only on it..)
We do need process_every_value here.

Speaking of which (the family of field operators, including all the postprocessors, that process a (single) prediction and process every value of list of references), why is EncodeLabels so much of an outsider?
There is a straightforward implementation thereof as a field operator. Why was not it used?

@dafnapension dafnapension force-pushed the no_dpath branch 2 times, most recently from b4a4346 to 3dc65c9 Compare March 20, 2024 21:58
@dafnapension dafnapension changed the title first draft for removal of dpath updated removal of dpath Mar 20, 2024
Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 93.58974% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 89.95%. Comparing base (93dc8d4) to head (3d0f6ff).

Files Patch % Lines
src/unitxt/dict_utils.py 90.38% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #680      +/-   ##
==========================================
+ Coverage   89.83%   89.95%   +0.11%     
==========================================
  Files          96       96              
  Lines        9201     9419     +218     
==========================================
+ Hits         8266     8473     +207     
- Misses        935      946      +11     

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

Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
@dafnapension dafnapension changed the title updated removal of dpath removal of dpath -- ready for review Mar 21, 2024
@dafnapension
Copy link
Collaborator Author

dafnapension commented Mar 21, 2024

@elronbandel , @yoavkatz , I suggest to remove the boolean parameter set_multiple from dict_set.
If the * comes at the end of the query, the same result can be achieved by assigning a whole vector (for arg value of dict_set()) to the parent of the * in the query; and if the * comes in the middle of the query, it is not always clear that the meaning was breaking the input value (the vector) into its individual components, and assign one component to each path, rather than the whole of value to each and every path.
It is also not clear what should happen with more than one * on the query.

Other than enjoying to play with it in the testing, I do not see any real value for this set_multiple parameter.

Again: the meaning thereof is clear if * comes at the end, but the same result is achievable by assigning the whole vector as one unit, to the parent of the * in the query.

The meaning thereof for * in the middle of the query was guessed by myself, and thus implemented.. You may see some examples in test_dict_utils.

I wrote extended docstring for dict_set. Please see if this looks clear enough.

Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Signed-off-by: dafnapension <dafnashein@yahoo.com>
Copy link
Member

@elronbandel elronbandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing Dafna! I think any comments on top of this implementation should be in a seperate PR as this one defentily satisfy its original goal! well done!

@elronbandel elronbandel merged commit 1ba3255 into main Mar 24, 2024
8 checks passed
lilacheden pushed a commit that referenced this pull request Mar 24, 2024
* first draft for removal of dpath

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* simpler home-made dpath

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* some fixes

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* start code coverage

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* extend code coverage

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* allow spaces in instance keys

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* remove dpath from requirement and expand code coverage

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* cover a few lines of code more

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* cover some more lines of code

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* EncodeLabel is back to instance rather than field operator.

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* live with set_multiple, and avoid premature end of recursive scan

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* cover some more

Signed-off-by: dafnapension <dafnashein@yahoo.com>

* cover more yet again

Signed-off-by: dafnapension <dafnashein@yahoo.com>

---------

Signed-off-by: dafnapension <dafnashein@yahoo.com>
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.

2 participants