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

Jgf writer simplification #1299

Merged

Conversation

jameshcorbett
Copy link
Member

@jameshcorbett jameshcorbett commented Sep 24, 2024

Now that #1293 has made the JGF reader smarter about defaults and the ion-R utility less verbose, the JGF match writer needs similar treatment.

Problem: the JGF reader understands edges as containment by default,
but the JGF writer outputs the value anyway even though it can be
quite verbose.

Suppress edge metadata for containment edges.
Problem: there are some extra branches in emit_vtx_base that could
be cut down.

Cut down the unnecessary branches.
@jameshcorbett jameshcorbett force-pushed the jgf-writer-simplification branch from 3ff7d5c to e7e50b6 Compare October 1, 2024 02:14
Problem: the JGF writer outputs a lot of default data for vertices.

Do not output default data, let the reader infer defaults.
@jameshcorbett jameshcorbett force-pushed the jgf-writer-simplification branch from e7e50b6 to 3e533c5 Compare October 1, 2024 02:22
@jameshcorbett jameshcorbett marked this pull request as ready for review October 1, 2024 02:23
@jameshcorbett jameshcorbett requested a review from trws October 1, 2024 02:24
@jameshcorbett
Copy link
Member Author

Ok @trws there is one failing test but I'm pretty sure I know what's wrong and it's just a matter of updating some JGFs after making sure everything looks sane (which is kind of slow since the JSON isn't formatted nicely so requires a lot of manual intervention). If the code looks good to you I could probably get the tests passing in time for the release tomorrow.

@jameshcorbett jameshcorbett mentioned this pull request Oct 1, 2024
Problem: the JGF writer no longer outputs some default data, but the
tests expect to find it.

Change the tests.
@jameshcorbett jameshcorbett force-pushed the jgf-writer-simplification branch from 3e533c5 to c1af690 Compare October 1, 2024 19:48
@jameshcorbett
Copy link
Member Author

Thanks! Setting MWP

@jameshcorbett jameshcorbett added the merge-when-passing mergify.io - merge PR automatically once CI passes label Oct 1, 2024
@mergify mergify bot merged commit 283b039 into flux-framework:master Oct 1, 2024
20 of 21 checks passed
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

Attention: Patch coverage is 67.74194% with 10 lines in your changes missing coverage. Please review.

Project coverage is 75.3%. Comparing base (d21d455) to head (c1af690).
Report is 35 commits behind head on master.

Files with missing lines Patch % Lines
resource/writers/match_writers.cpp 67.7% 10 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1299     +/-   ##
========================================
- Coverage    75.3%   75.3%   -0.1%     
========================================
  Files         111     111             
  Lines       15298   15297      -1     
========================================
- Hits        11530   11529      -1     
  Misses       3768    3768             
Files with missing lines Coverage Δ
resource/writers/match_writers.cpp 59.5% <67.7%> (-0.1%) ⬇️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-passing mergify.io - merge PR automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants