Skip to content

Fix icons#11

Open
Rogeryu1234 wants to merge 2 commits into
ZGainsforth:mainfrom
Rogeryu1234:FixGEMS
Open

Fix icons#11
Rogeryu1234 wants to merge 2 commits into
ZGainsforth:mainfrom
Rogeryu1234:FixGEMS

Conversation

@Rogeryu1234
Copy link
Copy Markdown
Collaborator

Fix github icons and title icons

@Rogeryu1234 Rogeryu1234 reopened this May 22, 2023
Copy link
Copy Markdown
Owner

@ZGainsforth ZGainsforth left a comment

Choose a reason for hiding this comment

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

I think this will require a change to stoichwebapp and stoichiometryfitter since we will be returning a plotly figure in both cases.

Comment thread StoichWeb.py
# @app.route('/')
# def notdash():
# df = pd.DataFrame({
# 'Fruit': ['Apples', 'Oranges', 'Bananas', 'Apples', 'Oranges',
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This part shouldn't be checked in.

ProtosolarChondricity = ProtosolarToFe

GEMSAtPct, GEMSAtPctSD = GEMSPlot(AtPct, ProtosolarToSi)
GEMSAtPct, GEMSAtPctSD= GEMSPlot(AtPct, ProtosolarToSi)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I prefer the space before the =


return Chondricity, ElementSigma, GEMSChiSqRed, DOF

def GEMSChiPlotWeb(AtPct, Protosolar):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

There should be only one ChiPlot function which is the same for web and wxpython. In both cases, we should return the plotly figure. If in wxpython, the fig.show auto-loads a browser, I think. If in web, then it can be embedded in the gui. But you don't want to fork code like this or a single change requires changes in two locations.

Copy link
Copy Markdown
Owner

@ZGainsforth ZGainsforth left a comment

Choose a reason for hiding this comment

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

I think this will require a change to stoichwebapp and stoichiometryfitter since we will be returning a plotly figure in both cases.

@ZGainsforth
Copy link
Copy Markdown
Owner

Alas, the icons and the GEMS phase commits are both in the same pull request. Because other work has been done in the meantime on the phase files, approving this pull request for the icon stuff would break the phase file stuff. You should go in and drop the phase commit, or else do a new pull request with just the icons.

Incidentally, this is a good example of why you want to think of commits and pull requests as atomic actions. Essentially, everything that is necessary for a certain feature is included, and nothing else is included. This allows features to be worked on in parallel without tripping on each other. With larger teams this becomes really important. It's less important on a project this size.

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