Skip to content

Conversation

pfebrer
Copy link
Contributor

@pfebrer pfebrer commented May 16, 2020

(This is just a copy of #205 using a new branch to allow smooth rebasing)

What is this

This pull request is to get visibility, so that people knows that this is under developement and I can (hopefully) get some feedback about it. Many syntax things still need to be changed, but the concept is already quite complete, so I'd like to get feedback on it to improve weak points.

For those who don't know, this project is an attempt to build a plotting framework and a graphical interface that can really display the great potential of sisl for processing scientific data.

How can I test it

It should be straightforward for sisl users to test it, I put everything needed in my branch (although I know that in production I will have to store it elsewhere) so that it is as simple as:

git clone -b GUI https://github.com/pfebrer96/sisl.git
cd sisl
pip install -e .[viz]

for anyone that would like to test it.

Where do I start

The docs are in https://sisl-siesta.xyz/sisl-docs/html/visualization/plotly and they contain several useful (I hope) notebooks that will introduce you to the framework.

Thanks in advance to any person that takes the time to check it out!

Cheers!

@pfebrer
Copy link
Contributor Author

pfebrer commented May 16, 2020

By the way, I'm almost done with everything I wanted to do for a first release. I just need to add a bit of documentation and write some missing tests and it will be ready.

@pfebrer pfebrer changed the title GUI fresh start after 270 commits to allow rebasing Sisl visualization module and graphical interface May 16, 2020
@pfebrer
Copy link
Contributor Author

pfebrer commented May 19, 2020

Hi!

I would say I'm satisfied for now, so it can be reviewed whenever you have time. There's a lot of things to review, so if you want we can have a call to guide you through it and discuss.

I would say also that the best way to check how it works would be to try to build a plot (see https://github.com/pfebrer96/sisl.viz-tutorials/blob/master/tutorials/DIY.ipynb).

Looking forward to your feedback!

@zerothi
Copy link
Owner

zerothi commented Jun 8, 2020

I am in the slow process of going through your code, should I create a new branch with edits that you can look through? Probably that would be easier given the large code?

@zerothi
Copy link
Owner

zerothi commented Jun 8, 2020

One thing that would be really good is minimum version requirements of the codes.
In particular plotly, flask and pandas?

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 8, 2020

I am in the slow process of going through your code, should I create a new branch with edits that you can look through? Probably that would be easier given the large code?

Great, thank you very much!

However you want, you can commit the changes directly in this branch if you want, all changes will be probably good :) But if you think a separate branch will be better you can do that too.

Please don't be mad when you get to the MultiplePlot's implementation of how to initialize multiple plots with multiprocessing. Apart from the bad syntax that I've not fixed yet, it may be unnecessarily complicated (I did it at the beggining), so if you can think of better ways to do it, you can straightforward change it (that's why I didn't spend much time trying to make the style right.

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 8, 2020

One thing that would be really good is minimum version requirements of the codes.
In particular plotly, flask and pandas?

Yes, I will check that!

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 8, 2020

I will mark each commented file as Done, to keep track of what I still need to address. I hope you don't mind about all the comments that you will get :)

@zerothi
Copy link
Owner

zerothi commented Jun 8, 2020

I will mark each commented file as Done, to keep track of what I still need to address. I hope you don't mind about all the comments that you will get :)

Perfectly fine, then I can also comment ;)

@zerothi
Copy link
Owner

zerothi commented Jun 8, 2020

Also, it would be really good if you could try and have a look at #227 together with this, ideally I would like to get that one in first ;)

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 8, 2020

Yes, I will install that branch and run all the tests 👍

@zerothi
Copy link
Owner

zerothi commented Jun 9, 2020

Is it ready for a 2nd review?

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 9, 2020

I'm changing the way plotables are handled and have not yet implemented NC/SOC for fatbands, but for the rest, it is 😅

@zerothi
Copy link
Owner

zerothi commented Jun 9, 2020

Also, change GUI to lowercase ;)
Thanks!!!

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 9, 2020

👍

@zerothi
Copy link
Owner

zerothi commented Jun 16, 2020

I am a bit lost in this PR, is it ready for a 2nd look?

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 16, 2020

Yees I was just fixing some things in the meantime. By the way, don't worry about ''' instead of """ in the docs, I will change it now myself.

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 16, 2020

Nooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo, I've messed up again :(. I really don't understand how this rebasing stuff should propagate from the sisl repo to my fork master and then to my branch.

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 16, 2020

Well the history seems clean. Is this good?

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 16, 2020

Ok, now I understand your confusion. I hadn't seen your PR. I just saw the comments branch on your repo and I looked at the file changes. So I missed your PR comment. I'm sorry. I will apply the rest of things you said and let you know when everything is done.

Sorry again!

@zerothi zerothi closed this Jun 16, 2020
@zerothi zerothi reopened this Jun 16, 2020
@zerothi
Copy link
Owner

zerothi commented Jun 16, 2020

Nooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo, I've messed up again :(. I really don't understand how this rebasing stuff should propagate from the sisl repo to my fork master and then to my branch.

Generally the workflow should be this:

  1. Your master should always *only follow the projects master (i.e. my master)
  2. You then do a branch and do development there
  3. Never, ever, do a merge from anywhere into your development branch (there are always exceptions ;))
  4. If new commits are made to master, then first ensure you don't have uncommitted changes, then do
git checkout master
git pull
git checkout <your dev branch>
git rebase master

this will send you through a sequence of problems where you basically need to fix merge conflicts by this recipe:

git status
<edit files that are problems, shown as red from above command>
git add <files that have been fixed>
git rebase --continue

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 18, 2020

Thanks for taking the time to explain it!

What I don't understand is: when you do git pull on master, what are you pulling exactly? Because I need to first rebase my master to your master, don't I? Or pulling from my master automatically pulls from your master?

@zerothi
Copy link
Owner

zerothi commented Jun 18, 2020

Thanks for taking the time to explain it!

What I don't understand is: when you do git pull on master, what are you pulling exactly? Because I need to first rebase my master to your master, don't I? Or pulling from my master automatically pulls from your master?

No, generally one should always keep your master in line with my master, never rebase, always only pull.

So never rebase master against anything :)

git pull ensures that you have the most recent commits on my master.

Probably what is confusing to you is that your origin is your own repo?
Mine, for instance, looks like this:

git remote -v                                           master
jb	https://github.com/jonaslb/sisl.git (fetch)
jb	https://github.com/jonaslb/sisl.git (push)
nils	https://github.com/juijan/sisl.git (fetch)
nils	https://github.com/juijan/sisl.git (push)
origin	git@github.com:zerothi/sisl.git (fetch)
origin	git@github.com:zerothi/sisl.git (push)
pol	https://github.com/pfebrer96/sisl.git (fetch)
pol	https://github.com/pfebrer96/sisl.git (push)
tf	https://github.com/tfrederiksen/sisl.git (fetch)
tf	https://github.com/tfrederiksen/sisl.git (push)

If you prefer to have your origin pointing to your own repo, you should do this:

git branch -D master
git checkout --track zerothi/master -b master

which will create the master branch to track my master branch (in case my remote is called zerothi)

@pfebrer
Copy link
Contributor Author

pfebrer commented Jun 18, 2020

Greaat, now I understand!

One last thing, what if I want to ignore things that are specific to my environment (e.g. a folder called .vscode and a link to the environment: .env), is this possible? That's why I wanted to modify the .gitignore in my master but I then saw that it gets transmitted to the pull requests.

@zerothi
Copy link
Owner

zerothi commented Jun 18, 2020

Greaat, now I understand!

One last thing, what if I want to ignore things that are specific to my environment (e.g. a folder called .vscode and a link to the environment: .env), is this possible? That's why I wanted to modify the .gitignore in my master but I then saw that it gets transmitted to the pull requests.

See here I think this should just work out-of-the-box ;)

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 6, 2020

I updated the docs in the do it yourself section (https://sisl-siesta.xyz/sisl-docs/html/visualization/plotly/#do-it-yourself). Hopefully they can help you understand the implementation more easily :)

@pfebrer
Copy link
Contributor Author

pfebrer commented Oct 14, 2020

Today I addressed (hopefully right) two issues that you were concerned about:

  • The gui's build is no longer in the repo. In 3b84488 I introduced sisl.viz.plotly.gui._update.update_gui(), which grabs the latest release from https://github.com/pfebrer96/sislGUIpublic, unzips it and stores it in sisl/sisl/viz/plotly/gui/build. So I guess this function can be called within CI before publishing the package.
  • There is the option to prevent automatic loading of the viz module. In 895291d I introduced the environment variable SISL_VIZ_AUTOLOAD, which controls whether sisl.viz is loaded on import sisl. You can always load it by doing sisl.load_viz().

Hope this brings us closer to the final merge :)

@zerothi
Copy link
Owner

zerothi commented Oct 21, 2020

Great thanks for this! I'll have a look at it ASAP.

This is a seed to open up sisl's visualization module to aiida users, which
use lots of different codes. Therefore, it could potentially drive a bunch of new users
to sisl if it is adopted. Only bands have an entry point for aiida nodes for now.
@zerothi
Copy link
Owner

zerothi commented Dec 10, 2020

This is now merged!

@zerothi zerothi closed this Dec 10, 2020
@pfebrer
Copy link
Contributor Author

pfebrer commented Dec 10, 2020

🎉 🎉 🎉

@pfebrer pfebrer deleted the GUI branch January 12, 2021 15:31
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