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 - PTT PFT #11

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

WIP - PTT PFT #11

wants to merge 6 commits into from

Conversation

gabknight
Copy link
Owner

No description provided.

Copy link

korbit-ai bot commented Oct 1, 2024

👋 I'm here to help you review your pull request. When you're ready for me to perform a review, you can comment anywhere on this pull request with this command: /korbit-review.

As a reminder, here are some helpful tips on how we can collaborate together:

  • To have me re-scan your pull request, simply re-invoke the /korbit-review command in a new comment.
  • You can interact with me by tagging @korbit-ai in any conversation in your pull requests.
  • On any comment I make on your code, please leave a 👍 if it is helpful and a 👎 if it is unhelpful. This will help me learn and improve as we work together
  • Lastly, to learn more, check out our Docs.

Copy link
Owner Author

@gabknight gabknight left a comment

Choose a reason for hiding this comment

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

@JesusMda Can you test with the suggested changes if the results improves?

@@ -449,6 +449,7 @@ cdef class PTTDirectionGetter(ProbabilisticDirectionGetter):
for i in range(1, len_streamlines):
if self.propagate():
# the propagation failed
stream_status = ENDPOINT
Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't think this change is correct. The status shouldn't be changed. When propagates fails, the status is TRACKPOINT and should remains like this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is fix below to deal with the trackpoint status


i += i_sub-1
copy_point(&streamline[i-1,0], point)
# stream_status[0] = sc.check_point_c(point)

current_wm_pve = 1.0 - sc.get_include(point) - sc.get_exclude(point)
Copy link
Owner Author

Choose a reason for hiding this comment

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

this block needs to check all generated points, no only le last point.

      # update max_wm_pve
        for j in range(i_sub):
            pve_j = (1.0 - sc.get_include_c(&sub_streamline[j, 0]) - sc.get_exclude_c(&sub_streamline[j, 0]))
            if pve_j > max_wm_pve:
                max_wm_pve = pve_j


current_wm_pve = 1.0 - sc.get_include(point) - sc.get_exclude(point)
if current_wm_pve > max_wm_pve:
max_wm_pve = current_wm_pve

if stream_status[0] == TRACKPOINT:
# The tracking continues normally
# print("Se sale con status TRACKPOINT", i_sub)
Copy link
Owner Author

Choose a reason for hiding this comment

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

line 272-275 can be removed, and line 281 should become
elif stream_status[0] == INVALIDPOINT or stream_status[0] == TRACKPOINT:
If the generate_streamline(.) reaches an invalid point or stops in the WM (i.e. TRACKPOINT), pft should be used.

line 276:
if (stream_status[0] == ENDPOINT

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