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

Added ability to change color palettes #574

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Added ability to change color palettes #574

wants to merge 27 commits into from

Conversation

Seanny123
Copy link
Collaborator

Added a modal dialogue to choose between a variety of palettes

I meant to make this a self-contained commit, but because it plays a lot with legends and whatnot, it contains #565 and #567

@Seanny123
Copy link
Collaborator Author

@tcstewar @jgosmann if any of you are in the reviewing mood, start here

@Seanny123
Copy link
Collaborator Author

Nvm, in my efforts to rebase, I accidentally lost some code.

@Seanny123
Copy link
Collaborator Author

Ready for review again if someone is feeling ambitious/bored.

@Seanny123
Copy link
Collaborator Author

Current issues:

  • Palette selection should show the palette to the right of the choice

n_lines=self.n_lines, synapse=0)

if getattr(self.config, "legend_labels") == []:
json = self.javascript_config(info, override={"legend_labels":self.def_legend_labels})
Copy link
Collaborator

Choose a reason for hiding this comment

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

???? Why not just do

info = dict(uid=id(self), label=self.label, 
                   n_lines=self.n_lines, synapse=0,
                   legend_labels=self.def_legend_labels)

and get rid of this whole override thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because the behaviour of javascript_config is to override everything with the defaults. It doesn't look to see if the defaults have been set to anything else. Is that it's intended behaviour or should the code be changed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I understand... all javascript_config does is go through the config entry for the item and just into a json output:

        for attr in self.config._clsparams.params:
            cfg[attr] = getattr(self.config, attr)
        return json.dumps(cfg)

what do you mean by overriding everything with the defaults?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll break it down why it over-writes by going through the above code with the example of legend_labels being set in dict argument to ["A", "B", "C"].

cfg is the dict I've passed in as an argument. The for-loop takes values from the default config, which is self.config._clsparams and adds it to cfg. The default config contains legend_labels = [], so when the for-loop runs, regardless of what the value that cfg has for legend_labels, it will be over-written by the default config. That is why ["A","B","C"] will be over-written to []. Does that make sense now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's basically equivalent to running this code:

cfg = dict(label="pants", n_lines=4, legend_labels=["A", "B", "C"])

default_conf = dict(min=-1, max=1, legend_labels=[])

for attr in default_conf.keys():
    cfg[attr] = default_conf[attr]
print(cfg)

@bjkomer
Copy link
Contributor

bjkomer commented Oct 22, 2015

I'm seeing the basal ganglia plots disappear when making a change in the editor, they might not be getting saved properly in the config.

Conflicts:
	nengo_gui/components/component.py
	nengo_gui/components/netgraph.py
	nengo_gui/components/value.py
	nengo_gui/static/components/netgraph_item.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants