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

Migrate some utils functionality from the variable_stars notebook into the utils package #82

Open
beckynevin opened this issue Dec 4, 2023 · 7 comments
Assignees

Comments

@beckynevin
Copy link
Collaborator

@ericdrosas87 I'm not quite done polishing the functions that will need to move to utilities, so I could use some help if you wanted to glance over them and make suggestions for things I/we could change to bring them in line with what is currently in utils.py. My current working branch is variable_stars and the notebook is variable_stars_becky.ipynb. The relevant functions are towards the bottom of the notebook: get_cutout_image and make_calexp_fig and remove_figure. Of course, if you think these don't need to be in utils and can stay here, I'm also open to having that discussion.

This is all taking place in the variable_stars branch, and I think it would be best to migrate the utilities before we merge this branch with main.

@ericdrosas87
Copy link
Contributor

@beckynevin Yep I will take a look at the notebook and leave me notes here

@ericdrosas87
Copy link
Contributor

ericdrosas87 commented Dec 5, 2023

I know you said you weren't done cleaning up the functions and so this might already be on your radar, but the remove_figure() function should include the references that globally scoped in the notebook as input parameters and there should be a return value.

@ericdrosas87
Copy link
Contributor

Otherwise looks good

@beckynevin
Copy link
Collaborator Author

I know you said you weren't done cleaning up the functions and so this might already be on your radar, but the remove_figure() function should include the references that globally scoped in the notebook as input parameters and there should be a return value.

I'm working on this now - do you know what gc.collect() does? I know it's the garbage collector but do we need to assign it and then run some sort of additional removal like everything = gc.collect() del everything?

@beckynevin
Copy link
Collaborator Author

I know you said you weren't done cleaning up the functions and so this might already be on your radar, but the remove_figure() function should include the references that globally scoped in the notebook as input parameters and there should be a return value.

I'm also wondering, do we actually need a remove_figure() function? Can't we just do a plt.close() within the create_figure functions?

@ericdrosas87
Copy link
Contributor

ericdrosas87 commented Dec 7, 2023

I'm working on this now - do you know what gc.collect() does? I know it's the garbage collector but do we need to assign it and then run some sort of additional removal like everything = gc.collect() del everything?

It looks like the only thing gc.collect() returns is the number of objects in memory it cleaned up, so I don't think it's necessary to capture the return the value from gc.collect()

I'm also wondering, do we actually need a remove_figure() function? Can't we just do a plt.close() within the create_figure functions?

I'm honestly not sure, but my gut feel is that calling plt.close() should be sufficient. Calling .close() eventually causes the figure to be popped out of a list of figures which should be sufficient to be picked up when gc.collect() is called.

@beckynevin
Copy link
Collaborator Author

beckynevin commented Mar 7, 2024

Moving a conversation @ericdrosas87 and I have been having on slack to here -

Essentially, the issue is that we have all of these plotting utilities that we'd like to migrate to utils.py. However, there's an experimental WCS option to many of these utilities. Our solution for now (suggested by @ericdrosas87 ):

It might be worth separating out as its own function. There’s no harm in leaving the function in the utils.py for now, but maybe include a print() statement in the function with a message stating that the function is experimental/deprecated for the time being just in case anyone comes across it and tries to use it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants