Add the capacity p-median example to the notebook

[rongboxu]

#381

Hi! I want to pull the request of adding the capacity p-median example. I know I need to create a new forked repository later, just want to share the updated version so that I can get some advice!

Also, I noticed this pull request also contains my previous commit of adding references, I am confused of it because I remembered I have pulled it last week. If there is nothing wrong with this pull request, please do let me know and I will modify it!

Also, it says the diff on notebooks is too large to show. I suppose it's because I add the formulation, and made it longer.

[jGaboardi]

Great work, @rongboxu! Let's discuss this in today's meetings.

[jGaboardi]

We will need to update your predefined facilities section due to our documentation confusion --> xref: #352, #384

[jGaboardi]

I think your added section to the notebook looks great. Can you update according to the issue and PR for predefined facilities above?

[jGaboardi]

@rongboxu Add the following text above cell 49:

However, in many real world applications there may already be existing facility locations with the goal being to add one or more new facilities. Here we will define facilites $y_0$ and $y_1$ as already existing (they must be present in the model solution). This will lead to a sub-optimal solution.

Important: The facilities in "predefined_loc" are a binary array where 1 means the associated location must appear in the solution.

See an example in the current version of the pmedian notebook on main above cell 24.

[jGaboardi]

CI failures are not related to this PR – see #386

[jGaboardi]

@rongboxu I think all substantive suggestions and comments have been addressed for the inclusion of KNearestPMedian! Good work on this. This final step for this PR is fixing the conflicts in notebooks/p-median.ipynb, which I noted how to begin that above.

[rongboxu]

@rongboxu I think all substantive suggestions and comments have been addressed for the inclusion of KNearestPMedian! Good work on this. This final step for this PR is fixing the conflicts in notebooks/p-median.ipynb, which I noted how to begin that above.

Thank you for checking and reviewing the code! I will soon deal with the conflict in that ipynb and update it! Thank you again for providing the link to that comment.

[rongboxu]

Hi James! I have added the corresponding part to the p-median.ipynb. However, it seems it still has conflicts?
https://github.com/pysal/spopt/pull/387/commits/48334a11bbcc7c42346164cd2da374f911199163

[jGaboardi]

Hi James! I have added the corresponding part to the p-median.ipynb. However, it seems it still has conflicts?
https://github.com/pysal/spopt/pull/387/commits/48334a11bbcc7c42346164cd2da374f911199163

Yeah, conflicts in notebooks can be tricky... As another step, how about trying to clear all of the notebook's output before saving and then commit and push up again? Let's see if that does the trick. If it does not, we may have to actually open the .ipynb as a text file and look for the conflict markers, like these.

[rongboxu]

Hi James! I have added the corresponding part to the p-median.ipynb. However, it seems it still has conflicts?
https://github.com/pysal/spopt/pull/387/commits/48334a11bbcc7c42346164cd2da374f911199163

Yeah, conflicts in notebooks can be tricky... As another step, how about trying to clear all of the notebook's output before saving and then commit and push up again? Let's see if that does the trick. If it does not, we may have to actually open the .ipynb as a text file and look for the conflict markers, like these.

Hi James! I try to look for the conflict markers, but I can't find any:
.
And I look back my previous commits, I can see it seems to replace many images with new name, which are very long:
.

I am not sure if these changes cause the conflict.

[jGaboardi]

Aycaramba, this merge conflict is getting tedious. We'll leave this PR open for now until Levi, Germano, or I can figure out what the heck is going on here.

[gegen07]

@jGaboardi conflicts on notebooks are cumbersome. So, I rebased the branch and, as expected, it resolved all the merge conflicts, by the way, there are 13 conflicts. You can see there are no conflicts on the rebased branch. However, I have no permission to make all these changes in @rongboxu repo.

[gegen07]

Is it added to dependencies or I missed something? It is not installing when using this branch.

[jGaboardi]

Yes, I noticed this as well when try to run the notebook locally. @rongboxu pointpats needs to be added to:

• the .ci/ environments
• pyproject.tml
• environment.yaml

[rongboxu]

@gegen07 Hi Germano! Thank you for making the rebased branch!! It is very very helpful! It doesn't have conflicts now. I wonder if I should rebase rongboxu:main branch, or should I create a new feature branch, and rebase this feature branch, and then create the PR based on this feature branch?

[gegen07]

I think you could rebase this branch, but be careful rebasing it because it is a hard change in the history commits. I recommend using a diff tool as Levi commented on the last meeting or a git GUI client as GitHub Desktop to manage the conflicts. To avoid future problems with rebase, you should copy the files of your PR to another location to secure your summer work.

[rongboxu]

I just created a new branch, and I solved three conflicts, now there are no conflicts on the new branch.
Thank you for providing the problems may happen when doing the rebase! I think I still need to rebase this main branch.
After I rebase this main branch locally, it shows that if I want to push, I have to force push origin. It seems to rewrite the commit history. I am not sure if I need to do the force push.

[gegen07]

That's great! Nice job!
I think you can resubmit the PR with the new feature branch to follow the git workflow/convention. When we want to submit a change to a project, the convention is creating a feature branch to submit a PR, we should not create a PR based on our main fork branch.
After you submit the PR using the feature branch, I think we can close this one and in your GSoC submission, you can reference both for documentation of the code review.

@jGaboardi @ljwolf agree?

[jGaboardi]

That works for me in the spirit of getting this done.

[ljwolf]

Fine with that!

[jGaboardi]

@rongboxu @gegen07 @ljwolf I think this can now be closed since #397 has been merged. Is this correct?

[rongboxu]

@rongboxu @gegen07 @ljwolf I think this can now be closed since #397 has been merged. Is this correct?

Yes! All the changes have been migrated to #397. This can be closed!