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

Issue 67 chemical change #68

Merged
merged 4 commits into from
Jan 11, 2025
Merged

Issue 67 chemical change #68

merged 4 commits into from
Jan 11, 2025

Conversation

tmushayahama
Copy link
Contributor

issues

#67
#51

Changes

  • Now molecules have oval/ellipse shape
  • Added the publication on molecular function
  • Removed additional colors on if BP Only, CC Only (@vanaukenk does this apply on pathway view, we can open ticket, so far there are only 2 colors molecule and one color whether it is activity unit, bp only, cc only).

Note, there is no publication on the Small Molecules panel, but there will be one if that small molecule appears in Activity panel. I believe this is correct behavior.
image

image

For Dev

@pkalita-lbl I had to change the function label inner div to add the evidence icons, so technically it is now no longer function label, but function-node and the label is now inside the term-node. I didn't change any terminology in case you are already using the css. But however, lemme know if this is okay or if there a way to throw the evidence icons just like on the node. Also didn't change the svg, my formatter did that on-save

cc @vanaukenk @kltm

@vanaukenk
Copy link

Thanks @tmushayahama @pkalita-lbl

Removed additional colors on if BP Only, CC Only (@vanaukenk does this apply on pathway view, we can open ticket, so far there are only 2 colors molecule and one color whether it is activity unit, bp only, cc only).

In theory, the pathway view should only be meaningful for process/pathway models. That said, though, it's nice to have the different color for BP and CC only, if nothing else for the visual cue that those nodes are not MFs or otherwise part of an activity flow. I'd be fine if we kept that distinction. Was there a particular reason you changed that?

Note, there is no publication on the Small Molecules panel, but there will be one if that small molecule appears in Activity panel. I believe this is correct behavior.

I'm not sure this is correct. All of the small molecules are related to activities, so it doesn't make sense to me that some have references in the small molecules section while others don't. For now, can you also add the relation to the MF and the reference just like you've done for the other two molecules (but note that 'has output' would need to use the inverse relation, 'output of' MF).

We said we would do more work on the right-hand panel on a future pathway view project, though, so if it's a lot of work to make the molecules/relations changes we can do that in the future.

Thanks.

@tmushayahama
Copy link
Contributor Author

tmushayahama commented Jan 11, 2025

@vanaukenk I noticed that the colors for "BP Only" and "CC Only" hadn't been tested. This is understandable since scenarios where only BP or CC occur are rare. However, the current implementation resulted in these elements being invisible due to bad contrast, as shown below:

image

To address this, I put the colors back, but opted for lighter shades to ensure visibility.

Regarding the genes panel, I only focused on fixing publication issue 51. I understand that the small molecules can be standalone entities or have incoming edges (object) or have outgoing edges or both. Unlike activity units, which the reference is on "enabled by" edge, small molecules do not have a comparable reference point. That is why it is on the outgoing edge like is_mall molecule

image

But we can discuss

@tmushayahama tmushayahama merged commit 1c0e4c6 into main Jan 11, 2025
1 check passed
@tmushayahama tmushayahama deleted the issue-67-chemical-change branch January 11, 2025 03:48
@vanaukenk
Copy link

Thanks for the additional info, @tmushayahama

I agree about the contrast between the text and the old BP and CC only colors. Lighter colors for those two types of nodes would definitely be better.

We'll discuss the right-hand panel more in future work. My main concern about the chemical reference icons is that it won't be obvious to a user why some chemicals have a reference associated with them and others don't, so I'd err on the side of including a reference for all of them or none.

I'll watch slack to see when the changes are on noctua-dev for testing.

Thanks!

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