-
Notifications
You must be signed in to change notification settings - Fork 47
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
Visualization Pass #228
Visualization Pass #228
Conversation
Nice! I'll be able to get a proper review first thing in the morning. What outstanding things do you have for it? |
This pull request introduces 2 alerts when merging b8c5dfd into 9a3aef5 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
Lots of things really, but good enough to get in. |
ba458df
to
69a4957
Compare
This pull request fixes 2 alerts when merging af42704 into 9a3aef5 - view on LGTM.com fixed alerts:
Comment posted by LGTM.com |
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.
This feature is awesome!
I have one question about the save
method an when it should or should not be used. I suspect its something I missed in how TorsionDrive works, but I want to make sure.
as_array : bool, optional | ||
Converts the returned values to NumPy arrays | ||
force : bool, optional | ||
Forces a requery if data is already present | ||
|
||
Returns | ||
------- | ||
success : bool |
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.
This is now a str
.
@@ -80,6 +80,7 @@ def add_specification(self, | |||
spec = TorsionDriveSpecification( | |||
name=lname, optimization_spec=optimization_spec, qc_spec=qc_spec, description=description) | |||
self.data.td_specs[lname] = spec | |||
self.save() |
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.
Why does the TDDaset's add_
method here call a save
back to the Fractal server? This seems to be different behavior from the other Collection
classes which don't invoke save
until its manually called or compute
is called?
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.
Yes, Procedure Datasets track their data by ObjectId as our querying is less exact for procedures. Result Datasets only need the molecule ObjectId and it can find the exact result it needs.
@@ -141,14 +142,18 @@ def add_entry(self, | |||
raise KeyError(f"Record {name} already in the dataset.") | |||
|
|||
self.data.records[lname] = record | |||
self.save() |
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.
Same question about save
here in a get
method
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.
This is an add_entry
we must call save here to update the record. Slower, but safer version of Dataset add_entry
, we do not expect tens of thousands of entries here.
Everything is fine here except for the minor return type issue. Please approve and merge, I will fix the string issue in another PR. |
Description
This is a first pass at Dataset visualization which to plot statistics in Juypter notebooks. We picked the Plotly graph library because it both produce nice interactive graphs and also has the Dash library built on top which will easily allow us to build exploration web apps with the same code.
Bar plot example:
Violin plot example:
Other items:
Dataset.get_history()
will not automatically perform a variety of queries to pull down all matching data.Random bits:
qcfractal-manager
config top level now long accepts additional arguments.start-periodic
flag as an optional arg.Dataset.units
.Dataset.units="eV"
would change all internal units to electron volts for example, fixes Dataset DataFrame units #208.Status