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

Heatmap updates #750

Merged
merged 29 commits into from
Jun 13, 2023
Merged

Heatmap updates #750

merged 29 commits into from
Jun 13, 2023

Conversation

loftusa
Copy link
Collaborator

@loftusa loftusa commented Apr 4, 2021

  • adds section in heatmap tutorial for hierarchy labels
  • adds ticklabels functionality in heatmap function that seaborn has
  • small bug fix and error checking changes

@netlify
Copy link

netlify bot commented Apr 4, 2021

❌ Deploy Preview for graspologic failed.

🔨 Explore the source changes: 50dcb7d

🔍 Inspect the deploy log: https://app.netlify.com/sites/graspologic/deploys/6124f6021d885900084b9af3

graspologic/plot/plot.py Outdated Show resolved Hide resolved
@loftusa loftusa requested review from bdpedigo and j1c April 4, 2021 19:49
@bdpedigo
Copy link
Collaborator

bdpedigo commented Apr 5, 2021

why make this a wrapper, as opposed to an option?

@loftusa
Copy link
Collaborator Author

loftusa commented Apr 5, 2021

why make this a wrapper, as opposed to an option?

I thought it was a little more clunky as an option, since turning it into an option implies that you can't use the center or cmap params anymore. I guess I could make the cmap parameter only take sets of two colors in the binary case?

I can make it an option if you'd prefer!

pytest.ini Outdated Show resolved Hide resolved
@loftusa loftusa requested a review from bdpedigo April 12, 2021 17:17
colors : list-like or np.ndarray
A list of exactly two colors to use for the heatmap.

colorbar_ticklabels : list-like
Copy link
Collaborator

Choose a reason for hiding this comment

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

if I don't want this, can it be turned off (like it can be for heatmap)

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like yes, can you add somewhere in docs?

@bdpedigo
Copy link
Collaborator

@loftusa what's the status of this PR, left a few comments above but wondering if we should push in or close?

@loftusa
Copy link
Collaborator Author

loftusa commented Aug 24, 2021

@loftusa what's the status of this PR, left a few comments above but wondering if we should push in or close?

I think probably binary_heatmap is unnecessary / overengineering, but the additions I made to the tutorial + the bug fixes are still useful. I'll just take that part out, keep the useful stuff, and re-request review.

@@ -12,6 +12,7 @@
heatmap,
pairplot,
pairplot_with_gmm,
binary_heatmap,
Copy link
Collaborator

Choose a reason for hiding this comment

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

dont need this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed, fixed

@loftusa
Copy link
Collaborator Author

loftusa commented Oct 28, 2021

@bdpedigo status on this? I saw approved, but I'm not authorized to merge

@loftusa loftusa mentioned this pull request Oct 28, 2021
@bdpedigo
Copy link
Collaborator

tests still werent passing

@loftusa
Copy link
Collaborator Author

loftusa commented Oct 29, 2021

tests still werent passing

looks like they broke yesterday with 44f372e. One sec

@bdpedigo
Copy link
Collaborator

bdpedigo commented Dec 7, 2021

@loftusa any interest in getting this merged? if not will close for the time being

@loftusa
Copy link
Collaborator Author

loftusa commented Dec 7, 2021

@loftusa any interest in getting this merged? if not will close for the time being

Yeah - the doc-building error was weird and I wasn't sure how to fix, then other stuff took priority...

TypeError: 'generator' object is not reversible

Exception occurred:
  File "/home/docs/checkouts/readthedocs.org/user_builds/graspologic/envs/750/lib/python3.8/site-packages/sphinx/util/nodes.py", line 94, in apply_source_workaround
    for classifier in reversed(node.parent.traverse(nodes.classifier)):
TypeError: 'generator' object is not reversible

@bdpedigo
Copy link
Collaborator

bdpedigo commented Dec 7, 2021

I dont see any current doc errors, just a format check?

@bdpedigo
Copy link
Collaborator

I dont see any current doc errors, just a format check?

bump @loftusa

@bdpedigo bdpedigo closed this Dec 13, 2021
@bdpedigo bdpedigo reopened this Dec 13, 2021
@bdpedigo
Copy link
Collaborator

oops didnt mean to hit close

@loftusa
Copy link
Collaborator Author

loftusa commented Jan 18, 2022

AttributeError: type object 'gensim._matutils.array' has no attribute '__reduce_cython__'

what the heck is this

@bdpedigo
Copy link
Collaborator

seeing if the issue fixed itself 🤞

@bdpedigo bdpedigo merged commit 4499f7c into graspologic-org:dev Jun 13, 2023
bdpedigo added a commit that referenced this pull request Sep 29, 2023
* Added Python 3.11 (#1039)

* add python 3.11

* fix numba dep warning

* add release note entry (#1040)

* Updated heatmap (#750)

* test pytest ini

* add binary_heatmap, updates to tutorial notebook

* test plotting

* re-add pytest.ini

* Update plot.py

Fix a few bugs

* add new tests

* Update setup.cfg

* remove binary_heatmap

* mpl version req back to normal

* remove binary_heatmap import

* black

* formatting fix

---------

Co-authored-by: Benjamin Pedigo <benjamindpedigo@gmail.com>

* Fixed bugs in type specifications from Numpy 1.25 release (#1047)

* attempted fix for matrix type erroring

* fix spec of csr_array

* add a random seed

* remove isspmatrix

* fix a type check

* black

* Added option for more efficient graph matching matrix operations (#1046)

* more efficient graph matching matrix operations

* fix formatting

* remove an unused import (unrelated)

* expose fast kwarg to user, docs

* run black formatter

* add typehint

* fix mypy

---------

Co-authored-by: bkj <ben@jataware.com>
Co-authored-by: bdpedigo <benjamindpedigo@gmail.com>

* Added an `ax` argument for `screeplot`

* Add ax argument to screeplot (now returns None)

* add a return value for screeplot()

* Refactored for best practices

* Fixed Matplotlib 3.8 compatibility issues (#1049)

* specify angle as kwarg

* try removing some maybe unnecessary code?

* fight with mypy

* black

* fix sorts

* try switch to explain

* fix intersphinx

* other instances of wrong tutorial intersphinx

* 3.3.0 release prep (#1050)

* add release notes

* bump version

---------

Co-authored-by: Alex Loftus <alexloftus2004@gmail.com>
Co-authored-by: Ben Johnson <bkj.322@gmail.com>
Co-authored-by: bkj <ben@jataware.com>
Co-authored-by: Prajwal Agrawal <61898798+kidkoder432@users.noreply.github.com>
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