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

Add MigrationGraph Renderable #267

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

hmlli
Copy link

@hmlli hmlli commented Jul 21, 2022

This PR is to address issue #254 .

TO DO:

  • Addition of a new file, crystal_toolkit.renderables.migration_graph
  • Aaddition of a new MPComponent, to crystal_toolkit.components.migration_graph
  • A small example app in crystal_toolkit.apps.examples so that this can be added to the test suite and documentation
  • Corresponding tests

@mkhorton
Copy link
Member

Hi @hmlli, let me know if there's been any progress on this or if you'd like to meet to discuss?

@hmlli
Copy link
Author

hmlli commented Aug 26, 2022

@mkhorton sure! I'm still trying to figure out the scheme but I would like to meet to talk about it. I'll ping you in slack

@lgtm-com
Copy link

lgtm-com bot commented Sep 1, 2022

This pull request introduces 1 alert when merging 7e251c4 into b56f413 - view on LGTM.com

new alerts:

  • 1 for Syntax error

@hmlli hmlli changed the title [WIP] Add MigrationGraph Component [WIP] Add MigrationGraph Renderable Sep 1, 2022
@hmlli
Copy link
Author

hmlli commented Sep 14, 2022

@mkhorton Hi Matt! I think this PR is ready as a first iteration for the MigrationGraph related contents. Feel free to run the example app to check out how it looks. Please lmk if anything else is needed : )

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Not Matt here but left a few quick comments to save @mkhorton some time.

@@ -0,0 +1,18 @@
Metadata-Version: 2.1
Copy link
Member

Choose a reason for hiding this comment

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

The files in crystal_toolkit.egg-info/* should be .gitignored.

Copy link
Member

Choose a reason for hiding this comment

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

Same with dist/crystal_toolkit-2021.4.29-py3.9.egg and anything else in dist/*.

app = dash.Dash(assets_folder=SETTINGS.ASSETS_PATH)

# create the MigrationGraph object
mg = loadfn("LiMnP2O7_mg.json")
Copy link
Member

Choose a reason for hiding this comment

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

Tried to run the example from the root dir with

python crystal_toolkit/apps/examples/migrationgraph.py

and got the error

FileNotFoundError: [Errno 2] No such file or directory: 'LiMnP2O7_mg.json'

To make the example robust w.r.t. cwd, you might want to change

- mg = loadfn("LiMnP2O7_mg.json")

+ import os

+ module_dir = os.path.dirname(os.path.abspath(__file__))

+ mg = loadfn(f"{module_dir}/LiMnP2O7_mg.json")

@@ -0,0 +1,94 @@
import json
Copy link
Member

Choose a reason for hiding this comment

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

Files in build/* should also be ignored, not committed. Might be worth purging these files from git history since they're quite large.

@hmlli
Copy link
Author

hmlli commented Sep 14, 2022

thanks @janosh for your help! All issues in the above conversations should be resolved. @mkhorton please let me know if anything else needs to be changed :)

@mkhorton
Copy link
Member

Thanks both, reviewing now :)

@mkhorton mkhorton changed the title [WIP] Add MigrationGraph Renderable Add MigrationGraph Renderable Jul 26, 2023
@mkhorton
Copy link
Member

Removed the [WIP] from this PR title, forgot this is in the review queue! Apologies for letting it sit.

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.

3 participants