nyx-space / nyx

Nyx is a high fidelity, fast, reliable and validated astrodynamics toolkit library written in Rust and available in Python
https://nyxspace.com
GNU Affero General Public License v3.0
159 stars 18 forks source link

frame_find_path_for_orientation seems broken #276

Closed cmleinz closed 7 months ago

cmleinz commented 7 months ago

Bug report

Describe the bug

Below is the code for the function in question.

    /// Returns the machine path of the ephemeris whose orientation is requested
    pub fn frame_find_path_for_orientation(&self, name: &str) -> Result<Vec<usize>, NyxError> {
        if self.frame_root.name == name {
            // Return an empty vector (but OK because we're asking for the root)
            Ok(Vec::new())
        } else {
            let path = Vec::new();
            Self::frame_find_path(name, &path, &self.frame_root)
        }
    }

    /// Seek a frame from its orientation name
    fn frame_find_path(
        frame_name: &str,
        cur_path: &[usize],
        f: &FrameTree,
    ) -> Result<Vec<usize>, NyxError> {
        if f.name == frame_name {
            Ok(cur_path.to_vec())
        } else if f.children.is_empty() {
            Err(NyxError::ObjectNotFound {
                needle: frame_name.to_string(),
                haystack: f.children.iter().map(|c| c.name.clone()).collect(),
            })
        } else {
            for child in &f.children {
                let this_path = cur_path.to_owned();   // <---- Error: Here we just convert the current path 
                                                       // to owned without mutating it in any way, then recurse
                let child_attempt = Self::frame_find_path(frame_name, &this_path, child);
                if let Ok(found_path) = child_attempt {
                    return Ok(found_path);
                }
            }
            // Could not find name in iteration, fail
            Err(NyxError::ObjectNotFound {
                needle: frame_name.to_string(),
                haystack: f.children.iter().map(|c| c.name.clone()).collect(),
            })
        }
    }

I've annotated in the code where I think the problem is. I think based on other similar functions this is intended to append the child index to the path? If that is the case I'm happy to address this.

ChristopherRabotin commented 7 months ago

Hi Caleb,

If you're using Nyx mostly for planetary positions and astrodynamics computation, I strongly recommend you switch to ANISE. It's thoroughly validated and matches all SPICE computations that have been implemented.

Since I've been working on ANISE for a few years now, I have not maintained the cosmic module in Nyx, and will replace it in full by ANISE as soon as I can.

cmleinz commented 7 months ago

Thank you for the information, we can close this out! I'm mostly interested in contributing to this kind of work as I find it very interesting, thanks for your work on both libraries!