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

WIP: Headbang re-work #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

thequackdaddy
Copy link

Hello,

If I understand correctly, the headbang triples spatial smoothing is not beign supported because the current one doesn't work too well in python 3. After reviewing, I think I may have stumbled across a few bugs that make getting the old unittests to pass very difficult.

I decided to re-write it. Please let me know if this is something you'd consider replacing. Right now, I've saved this in another module (it is easier that way because i work with python 3 and the 2to3 process makes it hard to save a diff only for the changing part of the file. I'd incorporate this in pysal.esda.smoothing.

I've followed the algorithm described in this paper.

Right now, I've only re-written the Headbanging_Triples algorithm. I haven't experimented with the Headbanging_Median_Rate class yet. (It may be we can fully recycle that.)

Please let me know if this is something of interest to this project. Happy to finish it, or close this PR if you don't want to maintain.

Right now, I'm missing docstrings (I probably could just copy from the old method... but there's a few small simplifications to the API I woudl argue for... mainly dropping the requirement for data and k as they are already provided by the spatial weight w.

Also, the current unittests use the old nose template. There's a lot happening in the setUp function that doesn't really do anything.

@thequackdaddy thequackdaddy changed the title Headbang re-work WIP: Headbang re-work Nov 13, 2017
@sjsrey
Copy link
Member

sjsrey commented Nov 16, 2017

@thequackdaddy thanks for this. certainly it is of interest if you want to finish it we will merge. modernizing/pruning the tests would be very helpful.

@thequackdaddy
Copy link
Author

@sjsrey I would be happy to continue. I'd likely need a rather thorough review of this... there's lots of potential assumptions and not extremely thorough examples... So its highly likely what I do will be somewhat incorrect. Not having a good worked example makes writing unittests challenging.

When I get some time, I'll experiment some more with it shortly. If you have other resources/documentation, please let me know.

@sjsrey
Copy link
Member

sjsrey commented Nov 17, 2017

Here is some more documentation: https://surveillance.cancer.gov/headbang/

@sjsrey sjsrey self-requested a review November 17, 2017 04:57
@sjsrey sjsrey self-assigned this Nov 17, 2017
@sjsrey
Copy link
Member

sjsrey commented Nov 17, 2017

@thequackdaddy this is looking good - i will get a closer review in the coming days. One thing that comes to mind is whether we can assume k from the w argument. For triples (the logic of headbanging) we need k>=3 and for some w objects this might not hold for all i. On the other hand, for i with k_i<3 we could go the extrapolation route, so maybe your idea of dropping k holds? Food for thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP work in progress (for discussion)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants