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

Updated code for compatibility with Julia 1.10 #1

Merged

Conversation

KoyanoBunsho
Copy link

@KoyanoBunsho KoyanoBunsho commented Feb 19, 2024

Dear Dr. Ludwig Schmidt

I hope this message finds you well.
I am Bunsho Koyano, a second year Ph.D student at The University of Tokyo, majoring in Computer Science.
I find the implementation within this repository amazing and believe it would greatly benefit from an update.

It appears that the code may not be fully compatible with the latest version of Julia.
I've made several updates to your codes for compatibility with Julia 1.10.
I intend to contribute positively to the project. Below is a summary of the changes I implemented:

.jl files

  1. Library Imports: Ensured all Julia libraries were imported correctly.
  2. Struct Syntax: Transitioned from the deprecated immutable syntax to struct.
  3. Array Initialization: Updated array initialization methods to use the Array{T}(undef, dims...) syntax.
  4. Element-wise Operations: Modified direct subtraction operations (-) to element-wise subtraction (. -) where applicable.
  5. select -> partialsort
  6. Linear Spacing: Replaced the deprecated linspace function with range.

.ipynb files

  1. JLD → JLD2
  2. blas_set_num_threads(1) → BLAS.set_num_threads(1)
  3. ASCIIString → String (https://discourse.julialang.org/t/asciistring-not-defined/9570)
  4. logspace → 10 .^ range (https://discourse.julialang.org/t/how-to-do-logspace-in-julia-v-1-0-x/17452)
  5. round → round.
  6. tic() and toq() → @time begin and end

Thank you for considering this contribution to your project. I'm looking forward to your feedback and am eager to help with other things or adjustments.

Best regards

Bunsho Koyano

@@ -175,7 +179,9 @@ function fit_histogram_merging(ys::Array{Float64, 1}, sigma::Float64, num_target

# Select the num_holdout_pieces'th largest error (counting from the largest
# error down) as threshold.
error_threshold = select(candidate_errors, max(0, length(candidate_pieces) - num_holdout_pieces + 1))
sorted_errors = sort(candidate_errors)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there no select in Julia any more? IIRC the select is O(n) while sort is O(n log n), but I may misremember.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for your feedback. You are right about the performance difference between sort (O(n log n)-time) and select (O(n)-time). Using sort could indeed worsen performance.

The select function was renamed to partialsort in this PR because one person reported the name was misleading.

I will use partialsort as follows:

error_threshold = partialsort(candidate_errors, max(0, length(candidate_pieces) - num_holdout_pieces + 1))

@ludwigschmidt
Copy link
Owner

Thank you for the PR! I haven't kept up with Julia unfortunately, so it's great to get updates to make it compatible with more recent versions :-)

I left a small comment regarding select vs sort, otherwise it looks good.

Updating the notebooks would be great as well if you get a chance (but no worries if not!).

@KoyanoBunsho
Copy link
Author

KoyanoBunsho commented Feb 24, 2024

Thank you for the PR! I haven't kept up with Julia unfortunately, so it's great to get updates to make it compatible with more recent versions :-)

I left a small comment regarding select vs sort, otherwise it looks good.

Updating the notebooks would be great as well if you get a chance (but no worries if not!).

I greatly appreciate your kind feedback. The notebooks are not compatible with the Julia codes I updated. I plan to upload the updated notebooks soon.

@ludwigschmidt
Copy link
Owner

Great thank you! Let me know when you're done with your changes here and I'll merge them.

@KoyanoBunsho
Copy link
Author

Great thank you! Let me know when you're done with your changes here and I'll merge them.

Certainly. After completing all the updates, I will promptly inform you. Additionally, I will provide a summarized list of the changes made in this PR.

@KoyanoBunsho
Copy link
Author

KoyanoBunsho commented Feb 24, 2024

I have successfully updated the .jl and .ipynb files to ensure compatibility with Julia 1.10.
I have also confirmed that all notebooks are functioning without issues on my iMac.

It is worth noting that there has been a change in the execution time on the notebook files because the machine spec is different.
The specs of my iMac are as follows:

  • Processor: 3.6 GHz 10-core Intel Core i9
  • Memory: 128 GB 2667 MHz DDR4
  • Graphics: AMD Radeon Pro 5700 XT 16 GB

I would like to express my gratitude again for your kind response on this pull request.

Best regards

Bunsho Koyano

@KoyanoBunsho
Copy link
Author

@ludwigschmidt

I have completed the updates to the .jl and .ipynb files. I have also described the changes made to the .ipynb files.

I sincerely appreciate your time in this matter.

Best regards

Bunsho Koyano

@ludwigschmidt
Copy link
Owner

Looks great, thank you!

@ludwigschmidt ludwigschmidt merged commit 9d1c1c1 into ludwigschmidt:master Feb 25, 2024
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