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.

Sign up for GitHub

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

Jump to bottom

#482: Improve and complete JSON writing #483

Merged
merged 14 commits into from
Jan 9, 2024
Merged

#482: Improve and complete JSON writing #483

merged 14 commits into from
Jan 9, 2024

Conversation

cwschilly
Copy link
Contributor

@cwschilly cwschilly commented Dec 13, 2023 •
edited
Loading

Fixes #482

This PR also deprecates the -1 option (to balance all phases). If no phase is specified, it defaults to balancing phase 0.

@cwschilly cwschilly linked an issue Dec 13, 2023 that may be closed by this pull request
Improve and complete JSON writing #482
Closed
5 tasks
@cwschilly cwschilly self-assigned this Dec 13, 2023
@cwschilly cwschilly marked this pull request as draft December 13, 2023 16:27
@cwschilly cwschilly marked this pull request as ready for review December 13, 2023 20:40
@cwschilly cwschilly marked this pull request as draft December 13, 2023 21:40
@cwschilly
Copy link
Contributor Author

cwschilly commented Dec 13, 2023 •
edited
Loading

I've found a bug in the code--putting in draft until I fix it

Update: fixed

cwschilly
cwschilly commented Dec 14, 2023
View reviewed changes
src/lbaf/Model/lbsObject.py
"""Assign any extraneous parameters."""
self.__unused_params = unused_params

def get_unused_params(self) -> dict:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the way we dynamically get all Object QOI, this will now list unused_params as a QOI

@cwschilly cwschilly marked this pull request as ready for review December 14, 2023 14:50
@cwschilly cwschilly force-pushed the 482-improve-and-complete-json-writing branch from 3ee28b7 to 9781956 Compare December 14, 2023 14:51
ppebay
ppebay approved these changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@ppebay ppebay left a comment

Choose a reason for hiding this comment

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

Good , especially with unit test. Thanks

@cwschilly cwschilly force-pushed the 482-improve-and-complete-json-writing branch from 9d69b6c to c9fb79f Compare December 19, 2023 14:58
@cwschilly cwschilly requested a review from ppebay December 19, 2023 16:34
@cwschilly
Copy link
Contributor Author

I am reworking the unit test to use multi-phase data generated by vt

@cwschilly cwschilly marked this pull request as draft December 19, 2023 19:05
@cwschilly
Copy link
Contributor Author

I'm running into an issue with assigning communication dicts to the correct rank. Originally, I was pairing communications with ranks by checking to see which object the comm is from (communications["from"]["id"]). Then I would compare that id to the ids from rank.get_object_ids(). If they matched, I would write out that communication on that rank.

However, it seems like a lot of these communications are sent from "home": 0, "id": 0, but there isn't an object (entity, in the JSON) that has id: 0 (meaning that there is never a match). Am I misunderstanding something?

@cwschilly cwschilly marked this pull request as ready for review January 4, 2024 16:43
@cwschilly cwschilly requested a review from ppebay January 4, 2024 16:43
ppebay
ppebay approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@ppebay ppebay left a comment

Choose a reason for hiding this comment

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

These are good changes.

pierrepebay
pierrepebay reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@pierrepebay pierrepebay left a comment

Choose a reason for hiding this comment

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

Looks good otherwise. I have tested it as well.

src/lbaf/IO/lbsVTDataWriter.py Outdated Show resolved Hide resolved
@ppebay ppebay requested review from ppebay and pierrepebay January 9, 2024 21:00
ppebay
ppebay approved these changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@ppebay ppebay left a comment

Choose a reason for hiding this comment

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

Sorry @lifflander @pierrepebay I inadvertently requested a new review from everyone albeit only one from me was needed. Please ignore the new request.

@cwschilly cwschilly merged commit fe9974b into develop Jan 9, 2024
8 checks passed
@cwschilly cwschilly deleted the 482-improve-and-complete-json-writing branch January 9, 2024 21:03
github-actions bot pushed a commit that referenced this pull request Jan 9, 2024
@cwschilly
#482: Improve and complete JSON writing (#483)
61a145a 
* #482: write out unused params, subphases, and beginnings of communications

* #482: use mp manager to use dicts and lists correctly

* #482: add if statements and fix failing tests

* #482: add unit test for communications data

* #482: clean up

* #482: add flexibilty for multiple phases

* #482: add output folder for communications test

* #482: finalize code, fix failing tests

* #482: add new test data and work on supporting multiple phases

* #482: fix multi-phase test, deprecate -1 option

* #482: remove print statements and imbalance.txt

* #482: fix failing test

* #482: add extra if statement to guard against inconsistent inputs

* #482: remove unused function
@cwschilly cwschilly mentioned this pull request Jun 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppebay ppebay ppebay approved these changes

@lifflander lifflander Awaiting requested review from lifflander

@pierrepebay pierrepebay Awaiting requested review from pierrepebay

Assignees

@cwschilly cwschilly

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve and complete JSON writing
3 participants
@cwschilly @ppebay @pierrepebay